Re: [Qemu-devel] [PATCH 01/14] ./block/iscsi/init.c

[ronnie sahlberg <ronniesahlberg@gmail.com>]

Thankyou.

On Sat, Dec 4, 2010 at 7:32 AM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
>
> You want the library to be GPL, not LGPL?

I have changed it to LGPLv3 for next submission.

>> +       /* use a "random" isid */
>> +       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?  Users 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.

>> +       if (iscsi->alias != NULL) {
>> +               free(discard_const(iscsi->alias));
>> +               iscsi->alias = NULL;
>> +       }
>
> All these if statements are unnecessary.  free(NULL) is a nop.

I have removed these if-statement from here, as well as for all other
files where similar constructs were used.

>
>> +       if (iscsi->fd != -1) {
>> +               iscsi_disconnect(iscsi);
>
> Perhaps disconnect before freeing struct members?

Yes. Done.

>> +       while ((pdu = iscsi->outqueue)) {
>> +               SLIST_REMOVE(&iscsi->outqueue, pdu);
>> +               pdu->callback(iscsi, SCSI_STATUS_CANCELLED, NULL,
>> +                             pdu->private_data);
>> +               iscsi_free_pdu(iscsi, pdu);
>> +       }
>> +       while ((pdu = iscsi->waitpdu)) {
>> +               SLIST_REMOVE(&iscsi->waitpdu, pdu);
>> +               pdu->callback(iscsi, SCSI_STATUS_CANCELLED, NULL,
>> +                             pdu->private_data);
>> +               iscsi_free_pdu(iscsi, pdu);
>> +       }
>
> Could these callbacks expect iscsi to still be valid?  A safer order
> would seem to be: disconnect, cancel all, free.

Yes. Done.


> +iscsi_set_error(struct iscsi_context *iscsi, const char *error_string, ...)
>> +{
>> +       va_list ap;
>> +       char *str;
>> +
>> +       va_start(ap, error_string);
>> +       if (vasprintf(&str, error_string, ap) < 0) {
>
> This function is available in GNU and BSD.  Not 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.

>
>> +               /* not much we can do here */
>
> You need to assign str = NULL here.  Otherwise its value is undefined
> on GNU systems.

Good catch. Done.

>
>> +       }
>> +       if (iscsi->error_string != NULL) {
>> +               free(iscsi->error_string);
>> +       }
>> +       iscsi->error_string = str;
>> +       va_end(ap);
>> +}
>> +
>> +
>> +char *
>
> const char *?

Done.

> Stefan
>


Thanks
Ronnie Sahlberg