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.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
 tools/vsh.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/tools/vsh.c b/tools/vsh.c
index eca312b4b..24ea45aa4 100644
--- a/tools/vsh.c
+++ b/tools/vsh.c
@@ -815,8 +815,7 @@ vshCommandFree(vshCmd *cmd)
  * Look up an option passed to CMD by NAME.  Returns 1 with *OPT set
  * to the option if found, 0 with *OPT set to NULL if the name is
  * valid and the option is not required, -1 with *OPT set to NULL if
- * the option is required but not present, and assert if NAME is not
- * valid (which indicates a programming error).  No error messages are
+ * the option is required but not present. No error messages are
  * issued if a value is returned.
  */
 static int
@@ -829,8 +828,7 @@ vshCommandOpt(const vshCmd *cmd, const char *name, vshCmdOpt **opt,
 
     /* See if option is valid and/or required.  */
     *opt = NULL;
-    while (valid) {
-        assert(valid->name);
+    while (valid && valid->name) {
         if (STREQ(name, valid->name))
             break;
         valid++;
-- 
2.13.6

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

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).

>Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>---
> tools/vsh.c | 6 ++----
> 1 file changed, 2 insertions(+), 4 deletions(-)
>
>diff --git a/tools/vsh.c b/tools/vsh.c
>index eca312b4b..24ea45aa4 100644
>--- a/tools/vsh.c
>+++ b/tools/vsh.c
>@@ -815,8 +815,7 @@ vshCommandFree(vshCmd *cmd)
>  * Look up an option passed to CMD by NAME.  Returns 1 with *OPT set
>  * to the option if found, 0 with *OPT set to NULL if the name is
>  * valid and the option is not required, -1 with *OPT set to NULL if
>- * the option is required but not present, and assert if NAME is not
>- * valid (which indicates a programming error).  No error messages are
>+ * the option is required but not present. No error messages are
>  * issued if a value is returned.
>  */
> static int
>@@ -829,8 +828,7 @@ vshCommandOpt(const vshCmd *cmd, const char *name, vshCmdOpt **opt,
>
>     /* See if option is valid and/or required.  */
>     *opt = NULL;
>-    while (valid) {
>-        assert(valid->name);
>+    while (valid && valid->name) {
>         if (STREQ(name, valid->name))
>             break;
>         valid++;
>-- 
>2.13.6
>
>--
>libvir-list mailing list
>libvir-list@redhat.com
>https://www.redhat.com/mailman/listinfo/libvir-list
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

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