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

[Paolo Bonzini]

Il 06/08/2012 10:24, address@hidden 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