[Qemu-devel] [PATCH] iSCSI: generate default initiator-name from vm-name

[Qemu-devel] [PATCH] iSCSI: generate default initiator-name from vm-name

[Ronnie Sahlberg]

List,

Please find a patch that updates the iSCSI block driver to automatically generate a 'unique' initiator-name based on the name given to the virtual machine.

Normally users will always have to set the initiator-name properly since LUN masking on the iscsi targets would almost always require this in common environments, but for test environments it can often be convenient to have the name set automatically and still have the name semi-unique across different VMs.


regards
ronnie sahlberg

[Qemu-devel] [PATCH] ISCSI: Pick default initiator-name based on the name of the VM

[Ronnie Sahlberg]

This patch updates the iscsi layer to automatically pick a
'unique' initiator-name based on the name of the vm in case the user
has not set an explicit iqn-name to use.

Save the -name of the vm so that we can use it in the default iscsi name.

Add a new variable that holds the name for the vm as specified by -name
and assign this name to the iscsi_name pointer gtom block/iscsi.c

This way we can thus create default names to use as the initiator name
based on the guest session.

If -name is not specified, iscsi will use a default initiator-name of
iqn.2008-11.org.linux-kvm

If -name is specified, then iscsi will use a default initiatorname of
iqn.2008-11.org.linux-kvm:<name>

Signed-off-by: Ronnie Sahlberg <ronniesahlberg@gmail.com>
---
 block/iscsi.c   |   16 +++++++++++++---
 qemu-doc.texi   |    5 +++++
 qemu-options.hx |    8 ++++++++
 vl.c            |    7 +++++++
 4 files changed, 33 insertions(+), 3 deletions(-)

diff --git a/block/iscsi.c b/block/iscsi.c
index 993a86d..abf60ee 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -28,6 +28,7 @@
 #include <arpa/inet.h>
 #include "qemu-common.h"
 #include "qemu-error.h"
+#include "sysemu.h"
 #include "block_int.h"
 #include "trace.h"
 #include "hw/scsi-defs.h"
@@ -40,6 +41,8 @@
 #include <hw/scsi-defs.h>
 #endif
 
+const char *iscsi_name;
+
 typedef struct IscsiLun {
     struct iscsi_context *iscsi;
     int lun;
@@ -896,23 +899,30 @@ static char *parse_initiator_name(const char *target)
     QemuOptsList *list;
     QemuOpts *opts;
     const char *name = NULL;
+    char *default_name;
+
+    if (asprintf(&default_name, "iqn.2008-11.org.linux-kvm%s%s",
+                 iscsi_name ? ":" : "",
+                 iscsi_name ? iscsi_name : "") == -1) {
+        default_name = (char *)"iqn.2008-11.org.linux-kvm";
+    }
 
     list = qemu_find_opts("iscsi");
     if (!list) {
-        return g_strdup("iqn.2008-11.org.linux-kvm");
+        return g_strdup(default_name);
     }
 
     opts = qemu_opts_find(list, target);
     if (opts == NULL) {
         opts = QTAILQ_FIRST(&list->head);
         if (!opts) {
-            return g_strdup("iqn.2008-11.org.linux-kvm");
+            return g_strdup(default_name);
         }
     }
 
     name = qemu_opt_get(opts, "initiator-name");
     if (!name) {
-        return g_strdup("iqn.2008-11.org.linux-kvm");
+        return g_strdup(default_name);
     }
 
     return g_strdup(name);
diff --git a/qemu-doc.texi b/qemu-doc.texi
index 84dad19..2360f7b 100644
--- a/qemu-doc.texi
+++ b/qemu-doc.texi
@@ -734,6 +734,11 @@ Various session related parameters can be set via special options, either
 in a configuration file provided via '-readconfig' or directly on the
 command line.
 
+If the initiator-name is not specified qemu will use a default name
+of 'iqn.2008-11.org.linux-kvm[:<name>'] where <name> is the name of the
+virtual machine.
+
+
 @example
 Setting a specific initiator name to use when logging in to the target
 -iscsi initiator-name=iqn.qemu.test:my-initiator
diff --git a/qemu-options.hx b/qemu-options.hx
index dc68e15..3335bcc 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -1893,6 +1893,11 @@ images for the guest storage. Both disk and cdrom images are supported.
 Syntax for specifying iSCSI LUNs is
 ``iscsi://<target-ip>[:<port>]/<target-iqn>/<lun>''
 
+By default qemu will use the iSCSI initiator-name
+'iqn.2008-11.org.linux-kvm[:<name>]' but this can also be set from the command
+line or a configuration file.
+
+
 Example (without authentication):
 @example
 qemu-system-i386 -iscsi initiator-name=iqn.2001-04.com.example:my-initiator \
@@ -1922,6 +1927,9 @@ DEF("iscsi", HAS_ARG, QEMU_OPTION_iscsi,
     "                iSCSI session parameters\n", QEMU_ARCH_ALL)
 STEXI
 
+iSCSI parameters such as username and password can also be specified via
+a configuration file. See qemu-doc for more information and examples.
+
 @item NBD
 QEMU supports NBD (Network Block Devices) both using TCP protocol as well
 as Unix Domain Sockets.
diff --git a/vl.c b/vl.c
index 8904db1..3320b7a 100644
--- a/vl.c
+++ b/vl.c
@@ -169,6 +169,10 @@ int main(int argc, char **argv)
 
 #define MAX_VIRTIO_CONSOLES 1
 
+#ifdef CONFIG_LIBISCSI
+extern const char *iscsi_name;
+#endif
+
 static const char *data_dir;
 const char *bios_name = NULL;
 enum vga_retrace_method vga_retrace_method = VGA_RETRACE_DUMB;
@@ -3074,6 +3078,9 @@ int main(int argc, char **argv, char **envp)
 			os_set_proc_name(p);
 		     }	
 		 }	
+#ifdef CONFIG_LIBISCSI
+                iscsi_name = qemu_name;
+#endif
                 break;
             case QEMU_OPTION_prom_env:
                 if (nb_prom_envs >= MAX_PROM_ENVS) {
-- 
1.7.3.1

Re: [Qemu-devel] [PATCH] ISCSI: Pick default initiator-name based on the name of the VM

[Paolo Bonzini]

Il 28/07/2012 02:44, Ronnie Sahlberg ha scritto:
> This patch updates the iscsi layer to automatically pick a
> 'unique' initiator-name based on the name of the vm in case the user
> has not set an explicit iqn-name to use.
> 
> Save the -name of the vm so that we can use it in the default iscsi name.
> 
> Add a new variable that holds the name for the vm as specified by -name
> and assign this name to the iscsi_name pointer gtom block/iscsi.c
> 
> This way we can thus create default names to use as the initiator name
> based on the guest session.
> 
> If -name is not specified, iscsi will use a default initiator-name of
> iqn.2008-11.org.linux-kvm
> 
> If -name is specified, then iscsi will use a default initiatorname of
> iqn.2008-11.org.linux-kvm:<name>
> 
> Signed-off-by: Ronnie Sahlberg <ronniesahlberg@gmail.com>
> ---
>  block/iscsi.c   |   16 +++++++++++++---
>  qemu-doc.texi   |    5 +++++
>  qemu-options.hx |    8 ++++++++
>  vl.c            |    7 +++++++
>  4 files changed, 33 insertions(+), 3 deletions(-)
> 
> diff --git a/block/iscsi.c b/block/iscsi.c
> index 993a86d..abf60ee 100644
> --- a/block/iscsi.c
> +++ b/block/iscsi.c
> @@ -28,6 +28,7 @@
>  #include <arpa/inet.h>
>  #include "qemu-common.h"
>  #include "qemu-error.h"
> +#include "sysemu.h"
>  #include "block_int.h"
>  #include "trace.h"
>  #include "hw/scsi-defs.h"
> @@ -40,6 +41,8 @@
>  #include <hw/scsi-defs.h>
>  #endif
>  
> +const char *iscsi_name;

As a rule of thumb, every #ifdef CONFIG_* increases the chances that
your patch is rejected. :)

Please add instead a qemu_get_vm_name() function and make it return NULL
in qemu-tool.c.

Paolo

>  typedef struct IscsiLun {
>      struct iscsi_context *iscsi;
>      int lun;
> @@ -896,23 +899,30 @@ static char *parse_initiator_name(const char *target)
>      QemuOptsList *list;
>      QemuOpts *opts;
>      const char *name = NULL;
> +    char *default_name;
> +
> +    if (asprintf(&default_name, "iqn.2008-11.org.linux-kvm%s%s",
> +                 iscsi_name ? ":" : "",
> +                 iscsi_name ? iscsi_name : "") == -1) {
> +        default_name = (char *)"iqn.2008-11.org.linux-kvm";
> +    }
>  
>      list = qemu_find_opts("iscsi");
>      if (!list) {
> -        return g_strdup("iqn.2008-11.org.linux-kvm");
> +        return g_strdup(default_name);
>      }
>  
>      opts = qemu_opts_find(list, target);
>      if (opts == NULL) {
>          opts = QTAILQ_FIRST(&list->head);
>          if (!opts) {
> -            return g_strdup("iqn.2008-11.org.linux-kvm");
> +            return g_strdup(default_name);
>          }
>      }
>  
>      name = qemu_opt_get(opts, "initiator-name");
>      if (!name) {
> -        return g_strdup("iqn.2008-11.org.linux-kvm");
> +        return g_strdup(default_name);
>      }
>  
>      return g_strdup(name);
> diff --git a/qemu-doc.texi b/qemu-doc.texi
> index 84dad19..2360f7b 100644
> --- a/qemu-doc.texi
> +++ b/qemu-doc.texi
> @@ -734,6 +734,11 @@ Various session related parameters can be set via special options, either
>  in a configuration file provided via '-readconfig' or directly on the
>  command line.
>  
> +If the initiator-name is not specified qemu will use a default name
> +of 'iqn.2008-11.org.linux-kvm[:<name>'] where <name> is the name of the
> +virtual machine.
> +
> +
>  @example
>  Setting a specific initiator name to use when logging in to the target
>  -iscsi initiator-name=iqn.qemu.test:my-initiator
> diff --git a/qemu-options.hx b/qemu-options.hx
> index dc68e15..3335bcc 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -1893,6 +1893,11 @@ images for the guest storage. Both disk and cdrom images are supported.
>  Syntax for specifying iSCSI LUNs is
>  ``iscsi://<target-ip>[:<port>]/<target-iqn>/<lun>''
>  
> +By default qemu will use the iSCSI initiator-name
> +'iqn.2008-11.org.linux-kvm[:<name>]' but this can also be set from the command
> +line or a configuration file.
> +
> +
>  Example (without authentication):
>  @example
>  qemu-system-i386 -iscsi initiator-name=iqn.2001-04.com.example:my-initiator \
> @@ -1922,6 +1927,9 @@ DEF("iscsi", HAS_ARG, QEMU_OPTION_iscsi,
>      "                iSCSI session parameters\n", QEMU_ARCH_ALL)
>  STEXI
>  
> +iSCSI parameters such as username and password can also be specified via
> +a configuration file. See qemu-doc for more information and examples.
> +
>  @item NBD
>  QEMU supports NBD (Network Block Devices) both using TCP protocol as well
>  as Unix Domain Sockets.
> diff --git a/vl.c b/vl.c
> index 8904db1..3320b7a 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -169,6 +169,10 @@ int main(int argc, char **argv)
>  
>  #define MAX_VIRTIO_CONSOLES 1
>  
> +#ifdef CONFIG_LIBISCSI
> +extern const char *iscsi_name;
> +#endif
> +
>  static const char *data_dir;
>  const char *bios_name = NULL;
>  enum vga_retrace_method vga_retrace_method = VGA_RETRACE_DUMB;
> @@ -3074,6 +3078,9 @@ int main(int argc, char **argv, char **envp)
>  			os_set_proc_name(p);
>  		     }	
>  		 }	
> +#ifdef CONFIG_LIBISCSI
> +                iscsi_name = qemu_name;
> +#endif
>                  break;
>              case QEMU_OPTION_prom_env:
>                  if (nb_prom_envs >= MAX_PROM_ENVS) {
> 

[Qemu-devel] [PATCH] ISCSI: Pick default initiator-name based on the name of the VM

[Ronnie Sahlberg]

Version 2 based on suggestions from Paolo B.
Use a function 'qemu_get_vm_name()' to pass the -name from qemu to
block/iscsi.c instead of #ifdefs

This patch updates the iscsi layer to automatically pick a
'unique' initiator-name based on the name of the vm in case the user
has not set an explicit iqn-name to use.

Save the -name of the vm so that we can use it in the default iscsi name.

Add a new variable that holds the name for the vm as specified by -name
and assign this name to the iscsi_name pointer gtom block/iscsi.c

This way we can thus create default names to use as the initiator name
based on the guest session.

If -name is not specified, iscsi will use a default initiator-name of
iqn.2008-11.org.linux-kvm

If -name is specified, then iscsi will use a default initiatorname of
iqn.2008-11.org.linux-kvm:<name>

Signed-off-by: Ronnie Sahlberg <ronniesahlberg@gmail.com>
---
 block/iscsi.c   |   14 +++++++++++---
 qemu-common.h   |    1 +
 qemu-doc.texi   |    5 +++++
 qemu-options.hx |    8 ++++++++
 qemu-tool.c     |    5 +++++
 vl.c            |    5 +++++
 6 files changed, 35 insertions(+), 3 deletions(-)

diff --git a/block/iscsi.c b/block/iscsi.c
index 993a86d..243496b 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -896,23 +896,31 @@ static char *parse_initiator_name(const char *target)
     QemuOptsList *list;
     QemuOpts *opts;
     const char *name = NULL;
+    char *default_name;
+    const char *iscsi_name = qemu_get_vm_name();
+
+    if (asprintf(&default_name, "iqn.2008-11.org.linux-kvm%s%s",
+                 iscsi_name ? ":" : "",
+                 iscsi_name ? iscsi_name : "") == -1) {
+        default_name = (char *)"iqn.2008-11.org.linux-kvm";
+    }
 
     list = qemu_find_opts("iscsi");
     if (!list) {
-        return g_strdup("iqn.2008-11.org.linux-kvm");
+        return g_strdup(default_name);
     }
 
     opts = qemu_opts_find(list, target);
     if (opts == NULL) {
         opts = QTAILQ_FIRST(&list->head);
         if (!opts) {
-            return g_strdup("iqn.2008-11.org.linux-kvm");
+            return g_strdup(default_name);
         }
     }
 
     name = qemu_opt_get(opts, "initiator-name");
     if (!name) {
-        return g_strdup("iqn.2008-11.org.linux-kvm");
+        return g_strdup(default_name);
     }
 
     return g_strdup(name);
diff --git a/qemu-common.h b/qemu-common.h
index 7c8dac8..702b410 100644
--- a/qemu-common.h
+++ b/qemu-common.h
@@ -358,6 +358,7 @@ bool buffer_is_zero(const void *buf, size_t len);
 void qemu_progress_init(int enabled, float min_skip);
 void qemu_progress_end(void);
 void qemu_progress_print(float delta, int max);
+const char *qemu_get_vm_name(void);
 
 #define QEMU_FILE_TYPE_BIOS   0
 #define QEMU_FILE_TYPE_KEYMAP 1
diff --git a/qemu-doc.texi b/qemu-doc.texi
index 84dad19..2360f7b 100644
--- a/qemu-doc.texi
+++ b/qemu-doc.texi
@@ -734,6 +734,11 @@ Various session related parameters can be set via special options, either
 in a configuration file provided via '-readconfig' or directly on the
 command line.
 
+If the initiator-name is not specified qemu will use a default name
+of 'iqn.2008-11.org.linux-kvm[:<name>'] where <name> is the name of the
+virtual machine.
+
+
 @example
 Setting a specific initiator name to use when logging in to the target
 -iscsi initiator-name=iqn.qemu.test:my-initiator
diff --git a/qemu-options.hx b/qemu-options.hx
index dc68e15..3335bcc 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -1893,6 +1893,11 @@ images for the guest storage. Both disk and cdrom images are supported.
 Syntax for specifying iSCSI LUNs is
 ``iscsi://<target-ip>[:<port>]/<target-iqn>/<lun>''
 
+By default qemu will use the iSCSI initiator-name
+'iqn.2008-11.org.linux-kvm[:<name>]' but this can also be set from the command
+line or a configuration file.
+
+
 Example (without authentication):
 @example
 qemu-system-i386 -iscsi initiator-name=iqn.2001-04.com.example:my-initiator \
@@ -1922,6 +1927,9 @@ DEF("iscsi", HAS_ARG, QEMU_OPTION_iscsi,
     "                iSCSI session parameters\n", QEMU_ARCH_ALL)
 STEXI
 
+iSCSI parameters such as username and password can also be specified via
+a configuration file. See qemu-doc for more information and examples.
+
 @item NBD
 QEMU supports NBD (Network Block Devices) both using TCP protocol as well
 as Unix Domain Sockets.
diff --git a/qemu-tool.c b/qemu-tool.c
index 318c5fc..64b5e88 100644
--- a/qemu-tool.c
+++ b/qemu-tool.c
@@ -30,6 +30,11 @@ struct QEMUBH
     void *opaque;
 };
 
+const char *qemu_get_vm_name(void)
+{
+    return NULL;
+}
+
 Monitor *cur_mon;
 
 int monitor_cur_is_qmp(void)
diff --git a/vl.c b/vl.c
index 8904db1..dd5716c 100644
--- a/vl.c
+++ b/vl.c
@@ -291,6 +291,11 @@ static struct {
     { .driver = "qxl-vga",              .flag = &default_vga       },
 };
 
+const char *qemu_get_vm_name(void)
+{
+    return qemu_name;
+}
+
 static void res_free(void)
 {
     if (boot_splash_filedata != NULL) {
-- 
1.7.3.1

Re: [Qemu-devel] [PATCH] ISCSI: Pick default initiator-name based on the name of the VM

[Paolo Bonzini]

Il 30/07/2012 11:03, Ronnie Sahlberg ha scritto:
> Version 2 based on suggestions from Paolo B.

This text goes after "---" so it doesn't become part of the commit message.

> Save the -name of the vm so that we can use it in the default iscsi name.
> 
> Add a new variable that holds the name for the vm as specified by -name
> and assign this name to the iscsi_name pointer gtom block/iscsi.c

This text is obsolete, and what is "gtom"? :)

Paolo

[Qemu-devel] [PATCH] ISCSI: Pick default initiator-name based on the name of the VM

[ronniesahlberg]

From: Ronnie Sahlberg <ronniesahlberg@gmail.com>

This patch updates the iscsi layer to automatically pick a 'unique' initiator-name based on the name of the vm in case the user has not set an explicit iqn-name to use.

Create a new function qemu_get_vm_name() that returns the name of the VM, if specified.

This way we can thus create default names to use as the initiator name based on the guest session.

If the VM is not named via the '-name' command line argument, the iscsi initiator-name used wiull simply be

    iqn.2008-11.org.linux-kvm

If a name for the VM was specified with the '-name' option, iscsi will use a default initiatorname of

    iqn.2008-11.org.linux-kvm:<name>

These names are just the default iscsi initiator name that qemu will generate/use only when the user has not set an explicit initiator name via the commandlines or config files.

Signed-off-by: Ronnie Sahlberg <ronniesahlberg@gmail.com>
---
 block/iscsi.c   |   14 +++++++++++---
 qemu-common.h   |    1 +
 qemu-doc.texi   |    5 +++++
 qemu-options.hx |    8 ++++++++
 qemu-tool.c     |    5 +++++
 vl.c            |    5 +++++
 6 files changed, 35 insertions(+), 3 deletions(-)

diff --git a/block/iscsi.c b/block/iscsi.c
index 993a86d..243496b 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -896,23 +896,31 @@ static char *parse_initiator_name(const char *target)
     QemuOptsList *list;
     QemuOpts *opts;
     const char *name = NULL;
+    char *default_name;
+    const char *iscsi_name = qemu_get_vm_name();
+
+    if (asprintf(&default_name, "iqn.2008-11.org.linux-kvm%s%s",
+                 iscsi_name ? ":" : "",
+                 iscsi_name ? iscsi_name : "") == -1) {
+        default_name = (char *)"iqn.2008-11.org.linux-kvm";
+    }
 
     list = qemu_find_opts("iscsi");
     if (!list) {
-        return g_strdup("iqn.2008-11.org.linux-kvm");
+        return g_strdup(default_name);
     }
 
     opts = qemu_opts_find(list, target);
     if (opts == NULL) {
         opts = QTAILQ_FIRST(&list->head);
         if (!opts) {
-            return g_strdup("iqn.2008-11.org.linux-kvm");
+            return g_strdup(default_name);
         }
     }
 
     name = qemu_opt_get(opts, "initiator-name");
     if (!name) {
-        return g_strdup("iqn.2008-11.org.linux-kvm");
+        return g_strdup(default_name);
     }
 
     return g_strdup(name);
diff --git a/qemu-common.h b/qemu-common.h
index f16079f..f9deca6 100644
--- a/qemu-common.h
+++ b/qemu-common.h
@@ -376,6 +376,7 @@ bool buffer_is_zero(const void *buf, size_t len);
 void qemu_progress_init(int enabled, float min_skip);
 void qemu_progress_end(void);
 void qemu_progress_print(float delta, int max);
+const char *qemu_get_vm_name(void);
 
 #define QEMU_FILE_TYPE_BIOS   0
 #define QEMU_FILE_TYPE_KEYMAP 1
diff --git a/qemu-doc.texi b/qemu-doc.texi
index f32e9e2..35cabbc 100644
--- a/qemu-doc.texi
+++ b/qemu-doc.texi
@@ -734,6 +734,11 @@ Various session related parameters can be set via special options, either
 in a configuration file provided via '-readconfig' or directly on the
 command line.
 
+If the initiator-name is not specified qemu will use a default name
+of 'iqn.2008-11.org.linux-kvm[:<name>'] where <name> is the name of the
+virtual machine.
+
+
 @example
 Setting a specific initiator name to use when logging in to the target
 -iscsi initiator-name=iqn.qemu.test:my-initiator
diff --git a/qemu-options.hx b/qemu-options.hx
index 5e7d0dc..47cb5bd 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -1897,6 +1897,11 @@ images for the guest storage. Both disk and cdrom images are supported.
 Syntax for specifying iSCSI LUNs is
 ``iscsi://<target-ip>[:<port>]/<target-iqn>/<lun>''
 
+By default qemu will use the iSCSI initiator-name
+'iqn.2008-11.org.linux-kvm[:<name>]' but this can also be set from the command
+line or a configuration file.
+
+
 Example (without authentication):
 @example
 qemu-system-i386 -iscsi initiator-name=iqn.2001-04.com.example:my-initiator \
@@ -1926,6 +1931,9 @@ DEF("iscsi", HAS_ARG, QEMU_OPTION_iscsi,
     "                iSCSI session parameters\n", QEMU_ARCH_ALL)
 STEXI
 
+iSCSI parameters such as username and password can also be specified via
+a configuration file. See qemu-doc for more information and examples.
+
 @item NBD
 QEMU supports NBD (Network Block Devices) both using TCP protocol as well
 as Unix Domain Sockets.
diff --git a/qemu-tool.c b/qemu-tool.c
index 318c5fc..64b5e88 100644
--- a/qemu-tool.c
+++ b/qemu-tool.c
@@ -30,6 +30,11 @@ struct QEMUBH
     void *opaque;
 };
 
+const char *qemu_get_vm_name(void)
+{
+    return NULL;
+}
+
 Monitor *cur_mon;
 
 int monitor_cur_is_qmp(void)
diff --git a/vl.c b/vl.c
index e71cb30..065aec2 100644
--- a/vl.c
+++ b/vl.c
@@ -293,6 +293,11 @@ static struct {
     { .driver = "qxl-vga",              .flag = &default_vga       },
 };
 
+const char *qemu_get_vm_name(void)
+{
+    return qemu_name;
+}
+
 static void res_free(void)
 {
     if (boot_splash_filedata != NULL) {
-- 
1.7.3.1

Re: [Qemu-devel] [PATCH] ISCSI: Pick default initiator-name based on the name of the VM

[Paolo Bonzini]

Il 06/08/2012 10:24, ronniesahlberg@gmail.com ha scritto:
> diff --git a/block/iscsi.c b/block/iscsi.c
> index 993a86d..243496b 100644
> --- a/block/iscsi.c
> +++ b/block/iscsi.c
> @@ -896,23 +896,31 @@ static char *parse_initiator_name(const char *target)
>      QemuOptsList *list;
>      QemuOpts *opts;
>      const char *name = NULL;
> +    char *default_name;
> +    const char *iscsi_name = qemu_get_vm_name();
> +
> +    if (asprintf(&default_name, "iqn.2008-11.org.linux-kvm%s%s",

asprintf is not portable, g_strdup_printf is.


> +                 iscsi_name ? ":" : "",
> +                 iscsi_name ? iscsi_name : "") == -1) {
> +        default_name = (char *)"iqn.2008-11.org.linux-kvm";
> +    }
>  
>      list = qemu_find_opts("iscsi");
>      if (!list) {
> -        return g_strdup("iqn.2008-11.org.linux-kvm");
> +        return g_strdup(default_name);
>      }
>  
>      opts = qemu_opts_find(list, target);
>      if (opts == NULL) {
>          opts = QTAILQ_FIRST(&list->head);
>          if (!opts) {
> -            return g_strdup("iqn.2008-11.org.linux-kvm");
> +            return g_strdup(default_name);
>          }
>      }
>  
>      name = qemu_opt_get(opts, "initiator-name");
>      if (!name) {
> -        return g_strdup("iqn.2008-11.org.linux-kvm");
> +        return g_strdup(default_name);
>      }
>  

Each of these strdup calls is leaking the original default_name.

>      return g_strdup(name);

iscsi_open is never going to free this, but neither is libiscsi.

Putting all this together, we need on top of your patch something like
this:

diff --git a/block/iscsi.c b/block/iscsi.c
index 243496b..69044b0 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -899,31 +899,27 @@ static char *parse_initiator_name(const char *target)
     char *default_name;
     const char *iscsi_name = qemu_get_vm_name();
 
-    if (asprintf(&default_name, "iqn.2008-11.org.linux-kvm%s%s",
-                 iscsi_name ? ":" : "",
-                 iscsi_name ? iscsi_name : "") == -1) {
-        default_name = (char *)"iqn.2008-11.org.linux-kvm";
-    }
+    default_name = g_strdup_printf(&default_name, "iqn.2008-11.org.linux-kvm%s%s",
+                                   iscsi_name ? ":" : "",
+                                   iscsi_name ? iscsi_name : "");
 
     list = qemu_find_opts("iscsi");
-    if (!list) {
-        return g_strdup(default_name);
-    }
-
-    opts = qemu_opts_find(list, target);
-    if (opts == NULL) {
-        opts = QTAILQ_FIRST(&list->head);
+    if (list) {
+        opts = qemu_opts_find(list, target);
         if (!opts) {
-            return g_strdup(default_name);
+            opts = QTAILQ_FIRST(&list->head);
+        }
+        if (opts) {
+            name = qemu_opt_get(opts, "initiator-name");
         }
     }
 
-    name = qemu_opt_get(opts, "initiator-name");
-    if (!name) {
-        return g_strdup(default_name);
+    if (name) {
+        g_free(default_name);
+        return g_strdup(name);
+    } else {
+        return default_name;
     }
-
-    return g_strdup(name);
 }
 
 /*
@@ -951,7 +947,7 @@ static int iscsi_open(BlockDriverState *bs, const char *filename, int flags)
         error_report("Failed to parse URL : %s %s", filename,
                      iscsi_get_error(iscsi));
         ret = -EINVAL;
-        goto failed;
+        goto out;
     }
 
     memset(iscsilun, 0, sizeof(IscsiLun));
@@ -962,13 +958,13 @@ static int iscsi_open(BlockDriverState *bs, const char *filename, int flags)
     if (iscsi == NULL) {
         error_report("iSCSI: Failed to create iSCSI context.");
         ret = -ENOMEM;
-        goto failed;
+        goto out;
     }
 
     if (iscsi_set_targetname(iscsi, iscsi_url->target)) {
         error_report("iSCSI: Failed to set target name.");
         ret = -EINVAL;
-        goto failed;
+        goto out;
     }
 
     if (iscsi_url->user != NULL) {
@@ -977,7 +973,7 @@ static int iscsi_open(BlockDriverState *bs, const char *filename, int flags)
         if (ret != 0) {
             error_report("Failed to set initiator username and password");
             ret = -EINVAL;
-            goto failed;
+            goto out;
         }
     }
 
@@ -985,13 +981,13 @@ static int iscsi_open(BlockDriverState *bs, const char *filename, int flags)
     if (parse_chap(iscsi, iscsi_url->target) != 0) {
         error_report("iSCSI: Failed to set CHAP user/password");
         ret = -EINVAL;
-        goto failed;
+        goto out;
     }
 
     if (iscsi_set_session_type(iscsi, ISCSI_SESSION_NORMAL) != 0) {
         error_report("iSCSI: Failed to set session type to normal.");
         ret = -EINVAL;
-        goto failed;
+        goto out;
     }
 
     iscsi_set_header_digest(iscsi, ISCSI_HEADER_DIGEST_NONE_CRC32C);
@@ -1012,7 +1008,7 @@ static int iscsi_open(BlockDriverState *bs, const char *filename, int flags)
         != 0) {
         error_report("iSCSI: Failed to start async connect.");
         ret = -EINVAL;
-        goto failed;
+        goto out;
     }
 
     while (!task.complete) {
@@ -1023,11 +1019,7 @@ static int iscsi_open(BlockDriverState *bs, const char *filename, int flags)
         error_report("iSCSI: Failed to connect to LUN : %s",
                      iscsi_get_error(iscsi));
         ret = -EINVAL;
-        goto failed;
-    }
-
-    if (iscsi_url != NULL) {
-        iscsi_destroy_url(iscsi_url);
+        goto out;
     }
 
     /* Medium changer or tape. We dont have any emulation for this so this must
@@ -1039,19 +1031,22 @@ static int iscsi_open(BlockDriverState *bs, const char *filename, int flags)
         bs->sg = 1;
     }
 
-    return 0;
+    ret = 0;
 
-failed:
+out:
     if (initiator_name != NULL) {
         g_free(initiator_name);
     }
     if (iscsi_url != NULL) {
         iscsi_destroy_url(iscsi_url);
     }
-    if (iscsi != NULL) {
-        iscsi_destroy_context(iscsi);
+
+    if (ret) {
+        if (iscsi != NULL) {
+            iscsi_destroy_context(iscsi);
+        }
+        memset(iscsilun, 0, sizeof(IscsiLun));
     }
-    memset(iscsilun, 0, sizeof(IscsiLun));
     return ret;
 }
 

Can you confirm that this is how the initiator name should be passed, so I can
split the above patch and commit it?

Paolo

Re: [Qemu-devel] [PATCH] ISCSI: Pick default initiator-name based on the name of the VM

[ronnie sahlberg]

Paolo,


On Mon, Aug 6, 2012 at 6:51 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 06/08/2012 10:24, ronniesahlberg@gmail.com ha scritto:
>> diff --git a/block/iscsi.c b/block/iscsi.c
>> index 993a86d..243496b 100644
>> --- a/block/iscsi.c
>> +++ b/block/iscsi.c
>> @@ -896,23 +896,31 @@ static char *parse_initiator_name(const char *target)
>>      QemuOptsList *list;
>>      QemuOpts *opts;
>>      const char *name = NULL;
>> +    char *default_name;
>> +    const char *iscsi_name = qemu_get_vm_name();
>> +
>> +    if (asprintf(&default_name, "iqn.2008-11.org.linux-kvm%s%s",
>
> asprintf is not portable, g_strdup_printf is.
>
>
>> +                 iscsi_name ? ":" : "",
>> +                 iscsi_name ? iscsi_name : "") == -1) {
>> +        default_name = (char *)"iqn.2008-11.org.linux-kvm";
>> +    }
>>
>>      list = qemu_find_opts("iscsi");
>>      if (!list) {
>> -        return g_strdup("iqn.2008-11.org.linux-kvm");
>> +        return g_strdup(default_name);
>>      }
>>
>>      opts = qemu_opts_find(list, target);
>>      if (opts == NULL) {
>>          opts = QTAILQ_FIRST(&list->head);
>>          if (!opts) {
>> -            return g_strdup("iqn.2008-11.org.linux-kvm");
>> +            return g_strdup(default_name);
>>          }
>>      }
>>
>>      name = qemu_opt_get(opts, "initiator-name");
>>      if (!name) {
>> -        return g_strdup("iqn.2008-11.org.linux-kvm");
>> +        return g_strdup(default_name);
>>      }
>>
>
> Each of these strdup calls is leaking the original default_name.
>
>>      return g_strdup(name);
>
> iscsi_open is never going to free this, but neither is libiscsi.
>
> Putting all this together, we need on top of your patch something like
> this:
>
> diff --git a/block/iscsi.c b/block/iscsi.c
> index 243496b..69044b0 100644
> --- a/block/iscsi.c
> +++ b/block/iscsi.c
> @@ -899,31 +899,27 @@ static char *parse_initiator_name(const char *target)
>      char *default_name;
>      const char *iscsi_name = qemu_get_vm_name();
>
> -    if (asprintf(&default_name, "iqn.2008-11.org.linux-kvm%s%s",
> -                 iscsi_name ? ":" : "",
> -                 iscsi_name ? iscsi_name : "") == -1) {
> -        default_name = (char *)"iqn.2008-11.org.linux-kvm";
> -    }
> +    default_name = g_strdup_printf(&default_name, "iqn.2008-11.org.linux-kvm%s%s",
> +                                   iscsi_name ? ":" : "",
> +                                   iscsi_name ? iscsi_name : "");
>
>      list = qemu_find_opts("iscsi");
> -    if (!list) {
> -        return g_strdup(default_name);
> -    }
> -
> -    opts = qemu_opts_find(list, target);
> -    if (opts == NULL) {
> -        opts = QTAILQ_FIRST(&list->head);
> +    if (list) {
> +        opts = qemu_opts_find(list, target);
>          if (!opts) {
> -            return g_strdup(default_name);
> +            opts = QTAILQ_FIRST(&list->head);
> +        }
> +        if (opts) {
> +            name = qemu_opt_get(opts, "initiator-name");
>          }
>      }
>
> -    name = qemu_opt_get(opts, "initiator-name");
> -    if (!name) {
> -        return g_strdup(default_name);
> +    if (name) {
> +        g_free(default_name);
> +        return g_strdup(name);
> +    } else {
> +        return default_name;
>      }
> -
> -    return g_strdup(name);
>  }
>
>  /*
> @@ -951,7 +947,7 @@ static int iscsi_open(BlockDriverState *bs, const char *filename, int flags)
>          error_report("Failed to parse URL : %s %s", filename,
>                       iscsi_get_error(iscsi));
>          ret = -EINVAL;
> -        goto failed;
> +        goto out;
>      }
>
>      memset(iscsilun, 0, sizeof(IscsiLun));
> @@ -962,13 +958,13 @@ static int iscsi_open(BlockDriverState *bs, const char *filename, int flags)
>      if (iscsi == NULL) {
>          error_report("iSCSI: Failed to create iSCSI context.");
>          ret = -ENOMEM;
> -        goto failed;
> +        goto out;
>      }
>
>      if (iscsi_set_targetname(iscsi, iscsi_url->target)) {
>          error_report("iSCSI: Failed to set target name.");
>          ret = -EINVAL;
> -        goto failed;
> +        goto out;
>      }
>
>      if (iscsi_url->user != NULL) {
> @@ -977,7 +973,7 @@ static int iscsi_open(BlockDriverState *bs, const char *filename, int flags)
>          if (ret != 0) {
>              error_report("Failed to set initiator username and password");
>              ret = -EINVAL;
> -            goto failed;
> +            goto out;
>          }
>      }
>
> @@ -985,13 +981,13 @@ static int iscsi_open(BlockDriverState *bs, const char *filename, int flags)
>      if (parse_chap(iscsi, iscsi_url->target) != 0) {
>          error_report("iSCSI: Failed to set CHAP user/password");
>          ret = -EINVAL;
> -        goto failed;
> +        goto out;
>      }
>
>      if (iscsi_set_session_type(iscsi, ISCSI_SESSION_NORMAL) != 0) {
>          error_report("iSCSI: Failed to set session type to normal.");
>          ret = -EINVAL;
> -        goto failed;
> +        goto out;
>      }
>
>      iscsi_set_header_digest(iscsi, ISCSI_HEADER_DIGEST_NONE_CRC32C);
> @@ -1012,7 +1008,7 @@ static int iscsi_open(BlockDriverState *bs, const char *filename, int flags)
>          != 0) {
>          error_report("iSCSI: Failed to start async connect.");
>          ret = -EINVAL;
> -        goto failed;
> +        goto out;
>      }
>
>      while (!task.complete) {
> @@ -1023,11 +1019,7 @@ static int iscsi_open(BlockDriverState *bs, const char *filename, int flags)
>          error_report("iSCSI: Failed to connect to LUN : %s",
>                       iscsi_get_error(iscsi));
>          ret = -EINVAL;
> -        goto failed;
> -    }
> -
> -    if (iscsi_url != NULL) {
> -        iscsi_destroy_url(iscsi_url);
> +        goto out;
>      }
>
>      /* Medium changer or tape. We dont have any emulation for this so this must
> @@ -1039,19 +1031,22 @@ static int iscsi_open(BlockDriverState *bs, const char *filename, int flags)
>          bs->sg = 1;
>      }
>
> -    return 0;
> +    ret = 0;
>
> -failed:
> +out:
>      if (initiator_name != NULL) {
>          g_free(initiator_name);
>      }
>      if (iscsi_url != NULL) {
>          iscsi_destroy_url(iscsi_url);
>      }
> -    if (iscsi != NULL) {
> -        iscsi_destroy_context(iscsi);
> +
> +    if (ret) {
> +        if (iscsi != NULL) {
> +            iscsi_destroy_context(iscsi);
> +        }
> +        memset(iscsilun, 0, sizeof(IscsiLun));
>      }
> -    memset(iscsilun, 0, sizeof(IscsiLun));
>      return ret;
>  }
>
>
> Can you confirm that this is how the initiator name should be passed, so I can
> split the above patch and commit it?
>


That looks good.
Thanks!

ronnie sahlberg