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

On Mon, Jul 23, 2018 at 6:21 PM, Michal Prívozník <mprivozn(a)redhat.com&gt; 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(a)redhat.com&gt; >> --- >> 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?