Re: [PATCH]virsh: track alias option and improve error message when option duplicate its alias

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




> -----Original Message-----
> From: Peter Krempa [mailto:pkrempa@xxxxxxxxxx]
> Sent: Wednesday, October 30, 2013 10:04 PM
> To: Eric Blake; Chen Hanxiao; libvir-list@xxxxxxxxxx
> Subject: Re:  [PATCH]virsh: track alias option and improve error message
> when option duplicate its alias
> 
> On 10/30/13 14:59, Eric Blake wrote:
> > On 10/30/2013 07:41 AM, Peter Krempa wrote:
> >
> >>>              if ((*opts_seen & (1 << i)) && opt->type != VSH_OT_ARGV)
> {
> >>> -                vshError(ctl, _("option --%s already seen"), name);
> >>> -                goto cleanup;
> >>> +                if ((*opts_seen & (1 << (i - 1)))) {
> >>> +                    vshError(ctl, _("option '--%s' duplicates its alias
> '--%s'"),
> >>> +                             cmd->opts[i].name,
> cmd->opts[i-1].name);
> >>
> >> This is not right. This code depends on the aliased option being right
> >> before the target option in the array describing the options. If you
> >> move the options in the array around you may get strange error messages.
> >
> > We already have a check baked into vshCmddefOptParse that guarantees
> > that all alias options appear in the list earlier than what the alias
> > expands to, so the rest of the code base is free to rely on that.
> >
> >>
> >> The idea is good, but you need to make sure that the code doesn't depend
> >> on ordering of the options in the array.
> >
> > I haven't looked closely at the patch, but we CAN depend on the order of
> > options within the array.
> 
> Either way, offset of "-1" to the current option can't be guaranteed in
> case we have for example multiple aliases for the same main command or
> just put some in between, when the aliases will be before the command
> but not _RIGHT_ before. (I actually tried it.)
> 

Thanks. v2 patch will solve this issue.

> Peter
> 




--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list




[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]