reading socket.c

I was reading some functions in socket.c (0.4.30), and I
have some 
questions, only little details w/o much importance

cherokee_read():
	line 732:
		if (unlikely (buf == NULL)) {
			char tmp[buf_size+1];
			len = gnutls_record_recv (socket->session, tmp,
buf_size);
		} else {

	char tmp[buf_size+1] is C99, not ANSI C and gcc implements
it using 
alloca(3). The manual page says it is not recommendable to
use alloca. I 
suggest:

		if (unlikely (buf == NULL)) {
			char *tmp=malloc(buf_size+1);
			if(!tmp)
				return ret_error;
			len = gnutls_record_recv (socket->session, tmp,
buf_size);
			free(tmp);
		} else {

or something similar

	line 753:
		*done=0;
		Well, in line 833: if(done!=NULL) *done=len; So, I think
we would 
check *done!=NULL in every assigment (or never check it)
		I have seen a lot of *done=0; But in some cases (e.g. 
socket->is_tls==TLS and len== GNUTLS_E_AGAIN) no
assignment is made.
		I suggest a *done=0; with check in the beginning of
cherokee_read and 
remove the others *done=0;
	(in cherokee_writev the same but with written)

	line 789: goto out; I Would do:

	if(TLS==socket->is_tls) {
		/*[...] (w/o goto)*/
	} else {
		/*Plain read
		*/
		/*[...]*/
	}

	if(done)
		*done=len;

	return ret_ok;

	and remove the goto
_______________________________________________
Cherokee mailing list
Cherokee0x50.org
http://www.alobbs.com/cgi-bin/mailman/listinfo/cherokee

Perki Pat wrote:

 > I was reading some functions in socket.c (0.4.30), and
I have some
 > questions, only little details w/o much importance

   Lets see, comments are always welcome!

 > cherokee_read():
 >     line 732:
 >         if (unlikely (buf == NULL)) {
 >             char tmp[buf_size+1];
 >             len = gnutls_record_recv
(socket->session, tmp, buf_size);
 >         } else {
 >
 > char tmp[buf_size+1] is C99, not ANSI C and gcc
implements it using
 > alloca(3). The manual page says it is not
recommendable to use
 > alloca.

   Good point!  That buf == NULL thing is there to implement
the
   lingering close mechanism, so it doesn't matter much
where we read
   the data actually.  I've added a new static array right
before the
   recv() that should be enough.

 >     line 753:
 > *done=0; Well, in line 833: if(done!=NULL) *done=len;
So, I think we
 > would check *done!=NULL in every assigment (or never
check it) I
 > have seen a lot of *done=0; But in some cases (e.g.
 > socket->is_tls==TLS and len== GNUTLS_E_AGAIN) no
assignment is made.
 > I suggest a *done=0; with check in the beginning of
cherokee_read
 > and remove the others *done=0;

   Another good point. I think it shouldn't touch *done if
it is not
   necessary, understanding for necessary that it read
something. I've
   fixed a few functions in which we were calling _read()
and _write()
   with uninitialized arguments, and I've also removed the
*ret
   assignments.

   These changes are committed in the change-set 195:

     http://www.0x5
0.org/bugs/changeset/195


   Thanks Perki! 

-- 
Greetings, alo.
_______________________________________________
Cherokee mailing list
Cherokee0x50.org
http://www.alobbs.com/cgi-bin/mailman/listinfo/cherokee