Message ID | 501F857A.9020608@redhat.com |
---|---|
State | New |
Headers | show |
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
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; }