[libvirt] [PATCH 2/2]virsh: remove attach-disk '--shareable' option

Chen Hanxiao chenhanxiao at cn.fujitsu.com
Tue Oct 15 09:09:22 UTC 2013



> -----Original Message-----
> From: Peter Krempa [mailto:pkrempa at redhat.com]
> Sent: Tuesday, October 15, 2013 4:50 PM
> To: Chen Hanxiao; libvir-list at redhat.com
> Subject: Re: [libvirt] [PATCH 2/2]virsh: remove attach-disk '--shareable'
option
> 
> On 10/15/13 05:54, Chen Hanxiao wrote:
> > From: Chen Hanxiao <chenhanxiao at cn.fujitsu.com>
> >
> > '--mode' option could set shareable tag already.
> > We do not need to duplicate options.
> >
> >      },
> > -    {.name = "shareable",
> > -     .type = VSH_OT_BOOL,
> > -     .help = N_("shareable between domains")
> > -    },
> 
> We can't remove this whole part. The correct approach is to use correct
> flags to hide it.

I checked enum ' Command Option Flags', it seems that we do not have
an option for 'existed but not for use'.
Could you please give me some hints for this?

> 
> >      {.name = "rawio",
> >       .type = VSH_OT_BOOL,
> >       .help = N_("needs rawio capability")
> > @@ -625,9 +621,6 @@ cmdAttachDisk(vshControl *ctl, const vshCmd *cmd)
> >      if (wwn)
> >          virBufferAsprintf(&buf, "  <wwn>%s</wwn>\n", wwn);
> >
> > -    if (vshCommandOptBool(cmd, "shareable"))
> > -        virBufferAddLit(&buf, "  <shareable/>\n");
> > -
> 
> NACK to this part. As DanPB mentioned, we need to remove the
> documentation, to discourage use of it, but we can't remove the argument
> as it would break backwards compatibility.
> 

If --mode could do all the same job. I don't know why '--shareable'
introduced.
One scenario I can imagine is to set <readonly/> and <shareable/> in one
command.

As Daniel mentioned, --shareable may be mistakenly introduced.
I think at least we should remove it from docs and command --help.
So we didn't lose compatibility and do the right way of using this command.

Thanks

> >      if (straddr) {
> >          if (str2DiskAddress(straddr, &diskAddr) != 0) {
> >              vshError(ctl, _("Invalid address."));
> 
> Peter
> 






More information about the libvir-list mailing list