[libvirt] [PATCH v2 1/2] virsh: Support alias in attach-disk

[Han Han]

On Mon, Jul 23, 2018 at 6:21 PM, Michal Prívozník <mprivozn at redhat.com>
wrote:

> On 07/15/2018 12:08 PM, Han Han wrote:
> > Add --alias to support custom disk alias in virsh attach-disk.
> > Report error if custom alias doesn't start with 'ua-'.
> >
> > Signed-off-by: Han Han <hhan at redhat.com>
> > ---
> >  tools/virsh-domain.c | 15 ++++++++++++++-
> >  tools/virsh.pod      |  3 ++-
> >  2 files changed, 16 insertions(+), 2 deletions(-)
> >
> > diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
> > index 8adec1d9b1..467417852e 100644
> > --- a/tools/virsh-domain.c
> > +++ b/tools/virsh-domain.c
> > @@ -290,6 +290,10 @@ static const vshCmdOptDef opts_attach_disk[] = {
> >       .type = VSH_OT_STRING,
> >       .help = N_("wwn of disk device")
> >      },
> > +    {.name = "alias",
> > +     .type = VSH_OT_STRING,
> > +     .help = N_("custom alias name of disk device")
> > +    },
> >      {.name = "rawio",
> >       .type = VSH_OT_BOOL,
> >       .help = N_("needs rawio capability")
> > @@ -574,7 +578,7 @@ cmdAttachDisk(vshControl *ctl, const vshCmd *cmd)
> >                  *subdriver = NULL, *type = NULL, *mode = NULL,
> >                  *iothread = NULL, *cache = NULL, *io = NULL,
> >                  *serial = NULL, *straddr = NULL, *wwn = NULL,
> > -                *targetbus = NULL;
> > +                *targetbus = NULL, *alias = NULL;
> >      struct DiskAddress diskAddr;
> >      bool isFile = false, functionReturn = false;
> >      int ret;
> > @@ -611,6 +615,7 @@ cmdAttachDisk(vshControl *ctl, const vshCmd *cmd)
> >          vshCommandOptStringReq(ctl, cmd, "wwn", &wwn) < 0 ||
> >          vshCommandOptStringReq(ctl, cmd, "address", &straddr) < 0 ||
> >          vshCommandOptStringReq(ctl, cmd, "targetbus", &targetbus) < 0 ||
> > +        vshCommandOptStringReq(ctl, cmd, "alias", &alias) < 0 ||
> >          vshCommandOptStringReq(ctl, cmd, "sourcetype", &stype) < 0)
> >          goto cleanup;
> >
> > @@ -680,6 +685,14 @@ cmdAttachDisk(vshControl *ctl, const vshCmd *cmd)
> >      if (serial)
> >          virBufferAsprintf(&buf, "<serial>%s</serial>\n", serial);
> >
> > +    if (alias) {
> > +        if (!STRPREFIX(alias, "ua-")) {
> > +            vshError(ctl, _("Custom alias name should start with ua-"));
> > +            goto cleanup;
> > +        }
>
> I don't think this belongs here. I'd let libvirt report an error. The
> reason for that is to have checks in one place rather than scattered
> around the code. For instance, imagine that one day we lift the
> restriction and let users define alias in a free form. Using this
> version of virsh (and connecting to new libvirtd) they will be unable to
> do so.
> Or the other way round - we allow only certain characters to be in user
> alias. You are not checking them here - you're relying on libvirtd doing
> so.
>
> It is reasonable that libvirtd check the alias name. As I know, currently
libvirtd
will ignore the customized alias not starting with 'ua-'. Will we keep
ignoring
it or report an error in libvirtd?

> The rest looks okay.
>
> Michal
>



-- 
Best regards,
-----------------------------------
Han Han
Quality Engineer
Redhat.

Email: hhan at redhat.com
Phone: +861065339333
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20180723/33755535/attachment-0001.htm>