[01/14] ./block/iscsi/init.c

From: Ronnie Sahlberg <ronniesahlberg@gmail.com>

iscsi  client library  : init.c
This file contains functions to create a iscsi context,
destroy a context, error reporting api, as well
as basic functions to manipulate properties of an iscsi context.

...

./block/iscsi/ contains a copy of a general purpose iscsi client
library which is aimed at providing a clientside api for iscsi
for both qemu/kvm as well as otther scsi related utilities.

As such, there is need to make merging across various consumers,
qemu/kvm being one of many here, as easy as possible when features
are added to the library.
As such, no consumer/qemu specific code is used in this library as well
as coding guidelined might not be adhered to 100%

It is the intention that this library will be useful for many
and that iscsi use spawned from this will flourish.

Signed-off-by: Ronnie Sahlberg <ronniesahlberg@gmail.com>
---
 block/iscsi/init.c |  215 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 215 insertions(+), 0 deletions(-)
 create mode 100644 block/iscsi/init.c

On Fri, Dec 3, 2010 at 11:09 AM,  <ronniesahlberg@gmail.com> wrote:
> @@ -0,0 +1,215 @@
> +/*
> +   Copyright (C) 2010 by Ronnie Sahlberg <ronniesahlberg@gmail.com>
> +
> +   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

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

Am 03.12.2010 22:23, schrieb ronnie sahlberg:
> 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.

Please use "LGPL 2.1 or later". IIRC, qemu has some parts that are GPL 2
only, and LGPL 3 isn't compatible with that.

Kevin