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

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



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


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