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

[Stefan Hajnoczi]

On Fri, Dec 3, 2010 at 11:09 AM,  <address@hidden> wrote:
> @@ -0,0 +1,215 @@
> +/*
> +   Copyright (C) 2010 by Ronnie Sahlberg <address@hidden>
> +
> +   This program is free software; you can redistribute it and/or modify
> +   it under the terms of the GNU General Public License as published by
> +   the Free Software Foundation; either version 2 of the License, or
> +   (at your option) any later version.

You want the library to be GPL, not LGPL?

> +struct iscsi_context *
> +iscsi_create_context(const char *initiator_name)
> +{
> +       struct iscsi_context *iscsi;
> +
> +       iscsi = malloc(sizeof(struct iscsi_context));
> +       if (iscsi == NULL) {
> +               return NULL;
> +       }
> +
> +       bzero(iscsi, sizeof(struct iscsi_context));
> +
> +       iscsi->initiator_name = strdup(initiator_name);
> +       if (iscsi->initiator_name == NULL) {
> +               free(iscsi);
> +               return NULL;
> +       }
> +
> +       iscsi->fd = -1;
> +
> +       /* 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.

> +       iscsi_set_random_isid(iscsi);
> +
> +       return iscsi;
> +}
> +
> +int
> +iscsi_set_random_isid(struct iscsi_context *iscsi)
> +{
> +       iscsi->isid[0] = 0x80;
> +       iscsi->isid[1] = random()&0xff;
> +       iscsi->isid[2] = random()&0xff;
> +       iscsi->isid[3] = random()&0xff;
> +       iscsi->isid[4] = 0;
> +       iscsi->isid[5] = 0;
> +
> +       return 0;
> +}

Is there an interface to set a specific isid value?  Users will
probably want to use a permanent value.

> +int
> +iscsi_destroy_context(struct iscsi_context *iscsi)
> +{
> +       struct iscsi_pdu *pdu;
> +
> +       if (iscsi == NULL) {
> +               return 0;
> +       }
> +       if (iscsi->initiator_name != NULL) {
> +               free(discard_const(iscsi->initiator_name));
> +               iscsi->initiator_name = NULL;
> +       }
> +       if (iscsi->target_name != NULL) {
> +               free(discard_const(iscsi->target_name));
> +               iscsi->target_name = NULL;
> +       }
> +       if (iscsi->alias != NULL) {
> +               free(discard_const(iscsi->alias));
> +               iscsi->alias = NULL;
> +       }

All these if statements are unnecessary.  free(NULL) is a nop.

> +       if (iscsi->fd != -1) {
> +               iscsi_disconnect(iscsi);

Perhaps disconnect before freeing struct members?

> +       }
> +
> +       if (iscsi->inbuf != NULL) {
> +               free(iscsi->inbuf);
> +               iscsi->inbuf = NULL;
> +               iscsi->insize = 0;
> +               iscsi->inpos = 0;
> +       }
> +
> +       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.

> +
> +       if (iscsi->error_string != NULL) {
> +               free(iscsi->error_string);
> +       }
> +
> +       free(iscsi);
> +
> +       return 0;
> +}
> +
> +
> +
> +void
> +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)?

> +               /* not much we can do here */

You need to assign str = NULL here.  Otherwise its value is undefined
on GNU systems.

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

const char *?

> +iscsi_get_error(struct iscsi_context *iscsi)
> +{
> +       return iscsi->error_string;
> +}

Stefan