Re: [libvirt] [PATCH v2 1/2] virsh: Support alias in attach-disk
On 07/23/2018 04:23 PM, Han Han wrote:
> On Mon, Jul 23, 2018 at 6:21 PM, Michal Prívozník
> 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
>>> ---
>>> 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", ) < 0 ||
>>> vshCommandOptStringReq(ctl, cmd, "address", ) < 0 ||
>>> vshCommandOptStringReq(ctl, cmd, "targetbus", ) < 0 ||
>>> +vshCommandOptStringReq(ctl, cmd, "alias", ) < 0 ||
>>> vshCommandOptStringReq(ctl, cmd, "sourcetype", ) < 0)
>>> goto cleanup;
>>>
>>> @@ -680,6 +685,14 @@ cmdAttachDisk(vshControl *ctl, const vshCmd *cmd)
>>> if (serial)
>>> virBufferAsprintf(, "%s\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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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
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
> > ---
> > 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", ) < 0 ||
> > vshCommandOptStringReq(ctl, cmd, "address", ) < 0 ||
> > vshCommandOptStringReq(ctl, cmd, "targetbus", ) < 0 ||
> > +vshCommandOptStringReq(ctl, cmd, "alias", ) < 0 ||
> > vshCommandOptStringReq(ctl, cmd, "sourcetype", ) < 0)
> > goto cleanup;
> >
> > @@ -680,6 +685,14 @@ cmdAttachDisk(vshControl *ctl, const vshCmd *cmd)
> > if (serial)
> > virBufferAsprintf(, "%s\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: h...@redhat.com
Phone: +861065339333
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 1/2] virsh: Support alias in attach-disk
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
> ---
> 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", ) < 0 ||
> vshCommandOptStringReq(ctl, cmd, "address", ) < 0 ||
> vshCommandOptStringReq(ctl, cmd, "targetbus", ) < 0 ||
> +vshCommandOptStringReq(ctl, cmd, "alias", ) < 0 ||
> vshCommandOptStringReq(ctl, cmd, "sourcetype", ) < 0)
> goto cleanup;
>
> @@ -680,6 +685,14 @@ cmdAttachDisk(vshControl *ctl, const vshCmd *cmd)
> if (serial)
> virBufferAsprintf(, "%s\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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
