From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=57069 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1POd6I-0005ku-52 for qemu-devel@nongnu.org; Fri, 03 Dec 2010 16:23:31 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1POd6G-0005PF-Hu for qemu-devel@nongnu.org; Fri, 03 Dec 2010 16:23:30 -0500 Received: from mail-qy0-f173.google.com ([209.85.216.173]:40076) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1POd6G-0005PB-C1 for qemu-devel@nongnu.org; Fri, 03 Dec 2010 16:23:28 -0500 Received: by qyk1 with SMTP id 1so1362053qyk.4 for ; Fri, 03 Dec 2010 13:23:28 -0800 (PST) MIME-Version: 1.0 In-Reply-To: References: <1291374593-17448-1-git-send-email-ronniesahlberg@gmail.com> <1291374593-17448-2-git-send-email-ronniesahlberg@gmail.com> Date: Sat, 4 Dec 2010 08:23:27 +1100 Message-ID: Subject: Re: [Qemu-devel] [PATCH 01/14] ./block/iscsi/init.c From: ronnie sahlberg Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefan Hajnoczi Cc: qemu-devel@nongnu.org Thankyou. On Sat, Dec 4, 2010 at 7:32 AM, Stefan Hajnoczi wrote: > > You want the library to be GPL, not LGPL? I have changed it to LGPLv3 for next submission. >> + =A0 =A0 =A0 /* use a "random" isid */ >> + =A0 =A0 =A0 srandom(getpid() ^ time(NULL)); > > The random number generator has global state and the library may > interfere if the program also uses the srandom() interface. ... > Is there an interface to set a specific isid value? =A0Users will > probably want to use a permanent value. I have removed the call to srandom() and instead initialize it to a 'random' ISID where the B+C fields are initialized to getpid()^time(NULL). I also added a public function iscsi_set_isid_random(iscsi, value) where a user can set the value explicitly. >> + =A0 =A0 =A0 if (iscsi->alias !=3D NULL) { >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 free(discard_const(iscsi->alias)); >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 iscsi->alias =3D NULL; >> + =A0 =A0 =A0 } > > All these if statements are unnecessary. =A0free(NULL) is a nop. I have removed these if-statement from here, as well as for all other files where similar constructs were used. > >> + =A0 =A0 =A0 if (iscsi->fd !=3D -1) { >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 iscsi_disconnect(iscsi); > > Perhaps disconnect before freeing struct members? Yes. Done. >> + =A0 =A0 =A0 while ((pdu =3D iscsi->outqueue)) { >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 SLIST_REMOVE(&iscsi->outqueue, pdu); >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 pdu->callback(iscsi, SCSI_STATUS_CANCELLED= , NULL, >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 pdu->private_d= ata); >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 iscsi_free_pdu(iscsi, pdu); >> + =A0 =A0 =A0 } >> + =A0 =A0 =A0 while ((pdu =3D iscsi->waitpdu)) { >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 SLIST_REMOVE(&iscsi->waitpdu, pdu); >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 pdu->callback(iscsi, SCSI_STATUS_CANCELLED= , NULL, >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 pdu->private_d= ata); >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 iscsi_free_pdu(iscsi, pdu); >> + =A0 =A0 =A0 } > > Could these callbacks expect iscsi to still be valid? =A0A safer order > would seem to be: disconnect, cancel all, free. Yes. Done. > +iscsi_set_error(struct iscsi_context *iscsi, const char *error_string, .= ..) >> +{ >> + =A0 =A0 =A0 va_list ap; >> + =A0 =A0 =A0 char *str; >> + >> + =A0 =A0 =A0 va_start(ap, error_string); >> + =A0 =A0 =A0 if (vasprintf(&str, error_string, ap) < 0) { > > This function is available in GNU and BSD. =A0Not sure what level of > portability you are targeting (e.g. what about Windows)? Posix for now. Win32 later, if/when someone needs it. The makefile corrently only builds this support for posix systems. > >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 /* not much we can do here */ > > You need to assign str =3D NULL here. =A0Otherwise its value is undefined > on GNU systems. Good catch. Done. > >> + =A0 =A0 =A0 } >> + =A0 =A0 =A0 if (iscsi->error_string !=3D NULL) { >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 free(iscsi->error_string); >> + =A0 =A0 =A0 } >> + =A0 =A0 =A0 iscsi->error_string =3D str; >> + =A0 =A0 =A0 va_end(ap); >> +} >> + >> + >> +char * > > const char *? Done. > Stefan > Thanks Ronnie Sahlberg