| Message ID | 20161208135527.GH26641@redhat.com |
|---|---|
| State | New |
| Headers | show |
Am 08.12.2016 um 14:55 hat Daniel P. Berrange geschrieben: > On Thu, Dec 08, 2016 at 02:23:05PM +0100, Kevin Wolf wrote: > > This adds blockdev-add support to the iscsi block driver. > > > > Note that this is only compile tested at this point. Jeff is going to > > take over from here and bring the series to a mergable state. > > > > Kevin Wolf (6): > > iscsi: Split URL into individual options > > iscsi: Handle -iscsi user/password in bdrv_parse_filename() > > iscsi: Add initiator-name option > > iscsi: Add header-digest option > > iscsi: Add timeout option > > iscsi: Add blockdev-add support > > > > block/iscsi.c | 342 ++++++++++++++++++++++++++++++--------------------- > > qapi/block-core.json | 74 ++++++++++- > > 2 files changed, 272 insertions(+), 144 deletions(-) > > This series works as well as my series does, once you apply this fix > for the crash bug I mention: > > diff --git a/block/iscsi.c b/block/iscsi.c > index 6a11cdd..320e56a 100644 > --- a/block/iscsi.c > +++ b/block/iscsi.c > @@ -1524,7 +1524,7 @@ static void iscsi_parse_iscsi_option(const char *target, QDict *options) > { > QemuOptsList *list; > QemuOpts *opts; > - const char *user, *initiator_name, *header_digest, *timeout; > + const char *user, *initiator_name, *header_digest, *timeout, *password, *password_secret; > > list = qemu_find_opts("iscsi"); > if (!list) { > @@ -1542,10 +1542,14 @@ static void iscsi_parse_iscsi_option(const char *target, QDict *options) > user = qemu_opt_get(opts, "user"); > if (user) { > qdict_set_default_str(options, "user", user); > - qdict_set_default_str(options, "password", > - qemu_opt_get(opts, "password")); > - qdict_set_default_str(options, "password-secret", > - qemu_opt_get(opts, "password-secret")); > + } > + password = qemu_opt_get(opts, "password"); > + if (password) { > + qdict_set_default_str(options, "password", password); > + } > + password_secret = qemu_opt_get(opts, "password-secret"); > + if (password_secret) { > + qdict_set_default_str(options, "password-secret", password_secret); > } Looks good to me. Though I would keep these inside the 'if (user)' block. The driver silently ignores password/password-secret if user isn't given (this is how it worked even before this patch). The interesting case where it makes a difference is if you give one option directly to the block driver and the other one with -iscsi. I don't think we want to allow that, both options should come from the same place. Kevin
diff --git a/block/iscsi.c b/block/iscsi.c index 6a11cdd..320e56a 100644 --- a/block/iscsi.c +++ b/block/iscsi.c @@ -1524,7 +1524,7 @@ static void iscsi_parse_iscsi_option(const char *target, QDict *options) { QemuOptsList *list; QemuOpts *opts; - const char *user, *initiator_name, *header_digest, *timeout; + const char *user, *initiator_name, *header_digest, *timeout, *password, *password_secret; list = qemu_find_opts("iscsi"); if (!list) { @@ -1542,10 +1542,14 @@ static void iscsi_parse_iscsi_option(const char *target, QDict *options) user = qemu_opt_get(opts, "user"); if (user) { qdict_set_default_str(options, "user", user); - qdict_set_default_str(options, "password", - qemu_opt_get(opts, "password")); - qdict_set_default_str(options, "password-secret", - qemu_opt_get(opts, "password-secret")); + } + password = qemu_opt_get(opts, "password"); + if (password) { + qdict_set_default_str(options, "password", password); + } + password_secret = qemu_opt_get(opts, "password-secret"); + if (password_secret) { + qdict_set_default_str(options, "password-secret", password_secret); } initiator_name = qemu_opt_get(opts, "initiator-name");