Re: [PATCH 02/11] vshCommandOpt: Relax check for valid options

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

 



On 11/08/2017 10:54 AM, Martin Kletzander wrote:
> On Tue, Nov 07, 2017 at 01:22:50PM +0100, Michal Privoznik wrote:
>> When trying to get an opt for command typed on the command line
>> we first check if command has such option. Because if it doesn't
>> it is a programming error. For instance: vshCommandOptBool(cmd,
>> "config") called from say cmdStart() doesn't make sense since
>> there's no --config for the start command. However, we will want
>> to have generic completers which are going to check if various
>> options are set. And so it can happen that we will check for
>> non-existent option for given command. Therefore, we need to
>> relax the check otherwise we will hit the assert() and don't get
>> anywhere.
>>
> 
> I would prefer keeping the assert there since it's such an easy check
> for that kind of programming error.  Is there no way to distinguish
> between calls from the completer?  If not, then I would rename it to
> vshCommandOptInternal(), add a bool argument, make vshCommandOpt() call
> it with one value and the completer with another one (I don't care if
> there's yet another function for that or if completers use the Internal
> one).

So now that I'm trying to implement what you suggested I came to realize
that it would be suboptimal. I mean, we have couple of functions for
looking up arguments. For instance: vshCommandOptInt(),
vshBlockJobOptionBandwidth(),  virshCommandOptDomain() to name a few
which eventually call vshCommandOpt(). Now, if I leave the assert() in
and make it optional (say based on a boolean arg), I'd need to write
alternative functions to all aforementioned so that they call
vshCommandOpt() with the boolean arg set to false. For instance:
virshCommandOptDomainUnsafe(), vshCommandOptIntUnsafe() or something.
That looks like too much work for very little gain. Therefore I'd like
to stick with my original proposal and just drop the assert.

Michal

--
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]
  Powered by Linux