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

[Michal Privoznik]

On 11/09/2017 05:22 PM, Martin Kletzander wrote:
> 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.

I'm afraid that wouldn't fly either. I mean, not every *Opt*() takes
ctl. For instance here's a snippet from virshDomainInterfaceCompleter()
(patch 10/11):

char **
virshDomainInterfaceCompleter(vshControl *ctl,
                              const vshCmd *cmd,
                              unsigned int flags)
{
    ...
    if (vshCommandOptBool(cmd, "config"))
        domainXMLFlags = VIR_DOMAIN_XML_INACTIVE;

    if (virshDomainGetXML(ctl, cmd, domainXMLFlags, &xmldoc, &ctxt) < 0)
        goto error;
    ...
}

While virshDomainGetXML() takes ctl, vshCommandOptBool() does not. But
what we can do is to have a boolean in vshCmd struct (which represents
*parsed* command), and depending on that boolean assert() would be
called or not. And since the struct is public, we don't need any special
functions for it. All that'd be needed is (in the cmdComplete - patch
06/11):

   if (!completed_list && opt && opt->completer) {
       vshCmd *partial = NULL;
       vshCommandStringParse(autoCompleteOpaque, rl_line_buffer, &partial);
+      partial->doNotCallAssert = true;
       completed_list = opt->completer(autoCompleteOpaque,
                                       partial,
                                       opt->completer_flags);
       vshCommandFree(partial);


Okay, let me implement this idea and see how it looks.

Michal