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

login
register
mail settings
Submitter ronniesahlberg@gmail.com
Date Dec. 3, 2010, 11:09 a.m.
Message ID <1291374593-17448-2-git-send-email-ronniesahlberg@gmail.com>
Download mbox | patch
Permalink /patch/74124/
State New
Headers show

Comments

ronniesahlberg@gmail.com - Dec. 3, 2010, 11:09 a.m.
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
Stefan Hajnoczi - Dec. 3, 2010, 8:32 p.m.
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
ronniesahlberg@gmail.com - Dec. 3, 2010, 9:23 p.m.
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
Kevin Wolf - Dec. 6, 2010, 4:09 p.m.
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

Patch

diff --git a/block/iscsi/init.c b/block/iscsi/init.c
new file mode 100644
index 0000000..c6ed347
--- /dev/null
+++ b/block/iscsi/init.c
@@ -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.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program; if not, see <http://www.gnu.org/licenses/>.
+*/
+
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <strings.h>
+#include <stdlib.h>
+#include <stdarg.h>
+#include <sys/types.h>
+#include <unistd.h>
+#include <time.h>
+#include "iscsi.h"
+#include "iscsi-private.h"
+#include "slist.h"
+
+
+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));
+	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;
+}
+
+int
+iscsi_set_alias(struct iscsi_context *iscsi, const char *alias)
+{
+	if (iscsi->is_loggedin != 0) {
+		iscsi_set_error(iscsi, "Already logged in when adding alias\n");
+		return -2;
+	}
+
+	if (iscsi->alias != NULL) {
+		free(discard_const(iscsi->alias));
+		iscsi->alias = NULL;
+	}
+
+	iscsi->alias = strdup(alias);
+	if (iscsi->alias == NULL) {
+		iscsi_set_error(iscsi, "Failed to allocate alias name\n");
+		return -3;
+	}
+
+	return 0;
+}
+
+int
+iscsi_set_targetname(struct iscsi_context *iscsi, const char *target_name)
+{
+	if (iscsi->is_loggedin != 0) {
+		iscsi_set_error(iscsi, "Already logged in when adding "
+				"targetname\n");
+		return -2;
+	}
+
+	if (iscsi->target_name != NULL) {
+		free(discard_const(iscsi->target_name));
+		iscsi->target_name = NULL;
+	}
+
+	iscsi->target_name = strdup(target_name);
+	if (iscsi->target_name == NULL) {
+		iscsi_set_error(iscsi, "Failed to allocate target name\n");
+		return -3;
+	}
+
+	return 0;
+}
+
+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;
+	}
+	if (iscsi->fd != -1) {
+		iscsi_disconnect(iscsi);
+	}
+
+	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);
+	}
+
+	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) {
+		/* not much we can do here */
+	}
+	if (iscsi->error_string != NULL) {
+		free(iscsi->error_string);
+	}
+	iscsi->error_string = str;
+	va_end(ap);
+}
+
+
+char *
+iscsi_get_error(struct iscsi_context *iscsi)
+{
+	return iscsi->error_string;
+}
+
+int
+iscsi_set_header_digest(struct iscsi_context *iscsi,
+			enum iscsi_header_digest header_digest)
+{
+	if (iscsi->is_loggedin) {
+		iscsi_set_error(iscsi, "trying to set header digest while "
+				"logged in\n");
+		return -2;
+	}
+
+	iscsi->want_header_digest = header_digest;
+
+	return 0;
+}
+
+int
+iscsi_is_logged_in(struct iscsi_context *iscsi)
+{
+	return iscsi->is_loggedin;
+}