[libvirt] [PATCH v2 0/2] Add alias support in sub-commands of virsh
Fix make syntax-check failure in v1. v1 link: https://www.redhat.com/archives/libvir-list/2018-July/msg00929.html Han Han (2): virsh: Support alias in attach-disk virsh: Support alias in attach-interface tools/virsh-domain.c | 30 ++++++++++++++++++++++++++++-- tools/virsh.pod | 7 +++++-- 2 files changed, 33 insertions(+), 4 deletions(-) -- 2.17.1
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; + } + virBufferAsprintf(&buf, "<alias name='%s'/>", alias); + } + if (wwn) virBufferAsprintf(&buf, "<wwn>%s</wwn>\n", wwn); diff --git a/tools/virsh.pod b/tools/virsh.pod index 771e99591e..4f0d12225a 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -3027,7 +3027,7 @@ expected. =item B<attach-disk> I<domain> I<source> I<target> [[[I<--live>] [I<--config>] | [I<--current>]] | [I<--persistent>]] [I<--targetbus bus>] [I<--driver driver>] [I<--subdriver subdriver>] [I<--iothread iothread>] -[I<--cache cache>] [I<--io io>] [I<--type type>] +[I<--cache cache>] [I<--io io>] [I<--type type>] [I<--alias alias>] [I<--mode mode>] [I<--sourcetype sourcetype>] [I<--serial serial>] [I<--wwn wwn>] [I<--rawio>] [I<--address address>] [I<--multifunction>] [I<--print-xml>] @@ -3050,6 +3050,7 @@ disk formats. I<type> can indicate I<lun>, I<cdrom> or I<floppy> as alternative to the disk default, although this use only replaces the media within the existing virtual cdrom or floppy device; consider using B<update-device> for this usage instead. +I<alias> can set user supplied alias. It should start with 'ua-'. I<mode> can specify the two specific mode I<readonly> or I<shareable>. I<sourcetype> can indicate the type of source (block|file) I<cache> can be one of "default", "none", "writethrough", "writeback", -- 2.17.1
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
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.
It is reasonable that libvirtd check the alias name. As I know, currently
On Mon, Jul 23, 2018 at 6:21 PM, Michal Prívozník <mprivozn@redhat.com> wrote: 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@redhat.com Phone: +861065339333
On 07/23/2018 04:23 PM, Han Han wrote:
On Mon, Jul 23, 2018 at 6:21 PM, Michal Prívozník <mprivozn@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@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
Add --alias to support custom alias in virsh attach-interface. 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 | 4 +++- 2 files changed, 17 insertions(+), 2 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 467417852e..7fb419f6b5 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -842,6 +842,10 @@ static const vshCmdOptDef opts_attach_interface[] = { .type = VSH_OT_STRING, .help = N_("model type") }, + {.name = "alias", + .type = VSH_OT_STRING, + .help = N_("custom alias name of interface device") + }, {.name = "inbound", .type = VSH_OT_STRING, .help = N_("control domain's incoming traffics") @@ -915,7 +919,7 @@ cmdAttachInterface(vshControl *ctl, const vshCmd *cmd) virDomainPtr dom = NULL; const char *mac = NULL, *target = NULL, *script = NULL, *type = NULL, *source = NULL, *model = NULL, - *inboundStr = NULL, *outboundStr = NULL; + *inboundStr = NULL, *outboundStr = NULL, *alias = NULL; virNetDevBandwidthRate inbound, outbound; virDomainNetType typ; int ret; @@ -945,6 +949,7 @@ cmdAttachInterface(vshControl *ctl, const vshCmd *cmd) vshCommandOptStringReq(ctl, cmd, "mac", &mac) < 0 || vshCommandOptStringReq(ctl, cmd, "script", &script) < 0 || vshCommandOptStringReq(ctl, cmd, "model", &model) < 0 || + vshCommandOptStringReq(ctl, cmd, "alias", &alias) < 0 || vshCommandOptStringReq(ctl, cmd, "inbound", &inboundStr) < 0 || vshCommandOptStringReq(ctl, cmd, "outbound", &outboundStr) < 0) goto cleanup; @@ -1042,6 +1047,14 @@ cmdAttachInterface(vshControl *ctl, const vshCmd *cmd) if (model != NULL) virBufferAsprintf(&buf, "<model type='%s'/>\n", model); + if (alias != NULL) { + if (!STRPREFIX(alias, "ua-")) { + vshError(ctl, _("Custom alias name should start with ua-")); + goto cleanup; + } + virBufferAsprintf(&buf, "<alias name='%s'/>\n", alias); + } + if (inboundStr || outboundStr) { virBufferAddLit(&buf, "<bandwidth>\n"); virBufferAdjustIndent(&buf, 2); diff --git a/tools/virsh.pod b/tools/virsh.pod index 4f0d12225a..4af21462e9 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -3085,7 +3085,7 @@ Likewise, I<--shareable> is an alias for I<--mode shareable>. [[[I<--live>] [I<--config>] | [I<--current>]] | [I<--persistent>]] [I<--target target>] [I<--mac mac>] [I<--script script>] [I<--model model>] [I<--inbound average,peak,burst,floor>] [I<--outbound average,peak,burst>] -[I<--managed>] [I<--print-xml>] +[I<--alias alias>] [I<--managed>] [I<--print-xml>] Attach a new network interface to the domain. @@ -3139,6 +3139,8 @@ I<bridge> type and only for Xen domains. B<--model> specifies the network device model to be presented to the domain. +B<alias> can set user supplied alias. It should start with 'ua-'. + B<--inbound> and B<--outbound> control the bandwidth of the interface. At least one from the I<average>, I<floor> pair must be specified. The other two I<peak> and I<burst> are optional, so -- 2.17.1
On 07/15/2018 12:08 PM, Han Han wrote:
Add --alias to support custom alias in virsh attach-interface. 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 | 4 +++- 2 files changed, 17 insertions(+), 2 deletions(-)
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 467417852e..7fb419f6b5 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -842,6 +842,10 @@ static const vshCmdOptDef opts_attach_interface[] = { .type = VSH_OT_STRING, .help = N_("model type") }, + {.name = "alias", + .type = VSH_OT_STRING, + .help = N_("custom alias name of interface device") + }, {.name = "inbound", .type = VSH_OT_STRING, .help = N_("control domain's incoming traffics") @@ -915,7 +919,7 @@ cmdAttachInterface(vshControl *ctl, const vshCmd *cmd) virDomainPtr dom = NULL; const char *mac = NULL, *target = NULL, *script = NULL, *type = NULL, *source = NULL, *model = NULL, - *inboundStr = NULL, *outboundStr = NULL; + *inboundStr = NULL, *outboundStr = NULL, *alias = NULL; virNetDevBandwidthRate inbound, outbound; virDomainNetType typ; int ret; @@ -945,6 +949,7 @@ cmdAttachInterface(vshControl *ctl, const vshCmd *cmd) vshCommandOptStringReq(ctl, cmd, "mac", &mac) < 0 || vshCommandOptStringReq(ctl, cmd, "script", &script) < 0 || vshCommandOptStringReq(ctl, cmd, "model", &model) < 0 || + vshCommandOptStringReq(ctl, cmd, "alias", &alias) < 0 || vshCommandOptStringReq(ctl, cmd, "inbound", &inboundStr) < 0 || vshCommandOptStringReq(ctl, cmd, "outbound", &outboundStr) < 0) goto cleanup; @@ -1042,6 +1047,14 @@ cmdAttachInterface(vshControl *ctl, const vshCmd *cmd) if (model != NULL) virBufferAsprintf(&buf, "<model type='%s'/>\n", model);
+ if (alias != NULL) { + if (!STRPREFIX(alias, "ua-")) { + vshError(ctl, _("Custom alias name should start with ua-")); + goto cleanup; + } + virBufferAsprintf(&buf, "<alias name='%s'/>\n", alias);
Same here. This check is not desired here. To extend my reasoning in 1/2 - we are not checking if provided MAC address really is MAC address. For instance: # attach-interface --print-xml fedora network default --mac blah Michal
On 07/15/2018 12:08 PM, Han Han wrote:
Fix make syntax-check failure in v1. v1 link: https://www.redhat.com/archives/libvir-list/2018-July/msg00929.html
Han Han (2): virsh: Support alias in attach-disk virsh: Support alias in attach-interface
tools/virsh-domain.c | 30 ++++++++++++++++++++++++++++-- tools/virsh.pod | 7 +++++-- 2 files changed, 33 insertions(+), 4 deletions(-)
I'm fixing all the issues I've found, ACKing and pushing. Might be worth providing news.xml entry though. Can you post a patch for that please? Michal
On Mon, Jul 23, 2018 at 6:21 PM, Michal Prívozník <mprivozn@redhat.com> wrote:
On 07/15/2018 12:08 PM, Han Han wrote:
Fix make syntax-check failure in v1. v1 link: https://www.redhat.com/archives/libvir-list/2018- July/msg00929.html
Han Han (2): virsh: Support alias in attach-disk virsh: Support alias in attach-interface
tools/virsh-domain.c | 30 ++++++++++++++++++++++++++++-- tools/virsh.pod | 7 +++++-- 2 files changed, 33 insertions(+), 4 deletions(-)
I'm fixing all the issues I've found, ACKing and pushing.
Might be worth providing news.xml entry though. Can you post a patch for that please?
Sure. The news patch link: https://www.redhat.com/archives/libvir-list/2018-July/msg01516.html
Michal
-- Best regards, ----------------------------------- Han Han Quality Engineer Redhat. Email: hhan@redhat.com Phone: +861065339333
participants (2)
-
Han Han -
Michal Prívozník