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 <address@hidden> wrote:
> 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?
>


That looks good.
Thanks!

ronnie sahlberg