On 10/15/13 11:09, Chen Hanxiao wrote: > > >> -----Original Message----- >> From: Peter Krempa [mailto:pkrempa@xxxxxxxxxx] >> Sent: Tuesday, October 15, 2013 4:50 PM >> To: Chen Hanxiao; libvir-list@xxxxxxxxxx >> Subject: Re: [PATCH 2/2]virsh: remove attach-disk '--shareable' > option >> >> On 10/15/13 05:54, Chen Hanxiao wrote: >>> From: Chen Hanxiao <chenhanxiao@xxxxxxxxxxxxxx> >>> >>> '--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? Hmmm, right. There probably isn't a suitable option as VSH_OT_ALIAS has to be used with the same type. > >> >>> {.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. I don't know either. But it was introduced and released already, thus we can't kill it or we could possibly break someone who is already using it. > 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. Well that's exactly what I was trying to say in my original review. > > Thanks > >>> if (straddr) { >>> if (str2DiskAddress(straddr, &diskAddr) != 0) { >>> vshError(ctl, _("Invalid address.")); >> Peter
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list