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

[Martin Kletzander]

On Thu, Nov 09, 2017 at 04:57:54PM +0100, Michal Privoznik wrote:
>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.
>

How about setting a boolean in @ctl instead?  If you don't like that either,
then keep what you have, I don't care that much about it.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20171109/0e4657ea/attachment-0001.sig>