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@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.

The rest looks okay.

Michal