Thankyou.
On Sat, Dec 4, 2010 at 7:32 AM, Stefan Hajnoczi <stefa...@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
