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

[Michal Prívozník]

On 07/23/2018 04:23 PM, Han Han wrote:
> 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?

Well, currently it reports an error if alias is specified and does not
fill all the requirements. Remember, having "ua-" prefix is just one of
the requirements.

Michal