Re: [Qemu-block] [Qemu-devel] [PATCH v2 2/7] iscsi: Handle -iscsi user/password in bdrv_parse_filename()

[Kevin Wolf]

Am 07.02.2017 um 11:13 hat Fam Zheng geschrieben:
> On Wed, 01/25 12:42, Jeff Cody wrote:
> > From: Kevin Wolf <address@hidden>
> > 
> > This splits the logic in the old parse_chap() function into a part that
> > parses the -iscsi options into the new driver-specific options, and
> > another part that actually applies those options (called apply_chap()
> > now).
> > 
> > Note that this means that username and password specified with -iscsi
> > only take effect when a URL is provided. This is intentional, -iscsi is
> > a legacy interface only supported for compatibility, new users should
> > use the proper driver-specific options.
> > 
> > Signed-off-by: Kevin Wolf <address@hidden>
> > Signed-off-by: Jeff Cody <address@hidden>
> > ---
> >  block/iscsi.c | 78 
> > +++++++++++++++++++++++++++++++++--------------------------
> >  1 file changed, 44 insertions(+), 34 deletions(-)
> > 
> > diff --git a/block/iscsi.c b/block/iscsi.c
> > index 97cff30..fc91d0f 100644
> > --- a/block/iscsi.c
> > +++ b/block/iscsi.c
> > @@ -1236,29 +1236,14 @@ retry:
> >      return 0;
> >  }
> >  
> > -static void parse_chap(struct iscsi_context *iscsi, const char *target,
> > +static void apply_chap(struct iscsi_context *iscsi, QemuOpts *opts,
> >                         Error **errp)
> >  {
> > -    QemuOptsList *list;
> > -    QemuOpts *opts;
> >      const char *user = NULL;
> >      const char *password = NULL;
> >      const char *secretid;
> >      char *secret = NULL;
> >  
> > -    list = qemu_find_opts("iscsi");
> > -    if (!list) {
> > -        return;
> > -    }
> > -
> > -    opts = qemu_opts_find(list, target);
> > -    if (opts == NULL) {
> > -        opts = QTAILQ_FIRST(&list->head);
> > -        if (!opts) {
> > -            return;
> > -        }
> > -    }
> > -
> >      user = qemu_opt_get(opts, "user");
> >      if (!user) {
> >          return;
> > @@ -1587,6 +1572,41 @@ out:
> >      }
> >  }
> >  
> > +static void iscsi_parse_iscsi_option(const char *target, QDict *options)
> > +{
> > +    QemuOptsList *list;
> > +    QemuOpts *opts;
> > +    const char *user, *password, *password_secret;
> > +
> > +    list = qemu_find_opts("iscsi");
> > +    if (!list) {
> > +        return;
> > +    }
> > +
> > +    opts = qemu_opts_find(list, target);
> > +    if (opts == NULL) {
> > +        opts = QTAILQ_FIRST(&list->head);
> > +        if (!opts) {
> > +            return;
> > +        }
> > +    }
> > +
> > +    user = qemu_opt_get(opts, "user");
> > +    if (user) {
> > +        qdict_set_default_str(options, "user", user);
> > +    }
> > +
> > +    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);
> > +    }
> > +}
> > +
> >  /*
> >   * We support iscsi url's on the form
> >   * iscsi://[<username>%<password>@]<host>[:<port>]/<targetname>/<lun>
> > @@ -1625,6 +1645,9 @@ static void iscsi_parse_filename(const char 
> > *filename, QDict *options,
> >      qdict_set_default_str(options, "lun", lun_str);
> >      g_free(lun_str);
> >  
> > +    /* User/password from -iscsi take precedence over those from the URL */
> > +    iscsi_parse_iscsi_option(iscsi_url->target, options);
> > +
> 
> Isn't more natural to let the local option take precedence over the global 
> one?

One would think so, but that's how the option work today, so we can't
change it without breaking compatibility. We can only newly define the
precedence of the new driver-specific options vs. the existing ones, and
there the local driver-specific ones do take precedence.

> >      if (iscsi_url->user[0] != '\0') {
> >          qdict_set_default_str(options, "user", iscsi_url->user);
> >          qdict_set_default_str(options, "password", iscsi_url->passwd);
> > @@ -1659,6 +1682,10 @@ static QemuOptsList runtime_opts = {
> >              .type = QEMU_OPT_STRING,
> >          },
> >          {
> > +            .name = "password-secret",
> > +            .type = QEMU_OPT_STRING,
> > +        },
> > +        {
> 
> I think this added password-secret is not the one looked up in
> iscsi_parse_iscsi_option(), which is from -iscsi
> (block/iscsi-opts.c:qemu_iscsi_opts). Is this intended? Or does it belong to a
> different patch?

It is the one that it put into the QDict by iscsi_parse_iscsi_option(),
which is supposed to be the value from -iscsi.

Why do you think it's not the one? Maybe I'm missing something.

Kevin