[libvirt] [PATCH] vshCommandOpt: Do more checking if skipChecks is set

Michal Privoznik posted 1 patch 6 years ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/c75db62ffe00c5feec30de27b7832915518a2f85.1519809651.git.mprivozn@redhat.com
Test syntax-check passed
tools/vsh.c | 20 ++++++++++----------
1 file changed, 10 insertions(+), 10 deletions(-)
[libvirt] [PATCH] vshCommandOpt: Do more checking if skipChecks is set
Posted by Michal Privoznik 6 years ago
Currently if cmd->skipChecks is set (done only from completers)
some basic checks are skipped because we're working over
partially parsed command. See a26ff63ae4 for more detailed
explanation. Anyway, the referenced commit was too aggressive in
disabling checks and effectively returned success even in clear
case of failure. For instance:

  # domif-getlink --interface <TAB><TAB>

causes virshDomainInterfaceCompleter() to be called, which calls
virshDomainGetXML() which eventually calls
vshCommandOptStringReq(.., name = "domain"); The --domain
argument is required for the command and if not present -1 should
be returned to tell the caller the argument was not found. Well,
zero is returned meaning the argument was not found but it's not
required either.

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

diff --git a/tools/vsh.c b/tools/vsh.c
index 37c292a03..73ec007e5 100644
--- a/tools/vsh.c
+++ b/tools/vsh.c
@@ -817,18 +817,17 @@ vshCommandOpt(const vshCmd *cmd, const char *name, vshCmdOpt **opt,
     /* See if option is valid and/or required.  */
     *opt = NULL;
 
-    if (!cmd->skipChecks) {
-        while (valid && valid->name) {
-            if (STREQ(name, valid->name))
-                break;
-            valid++;
-        }
+    while (valid && valid->name) {
+        if (STREQ(name, valid->name))
+            break;
+        valid++;
+    }
 
+    if (!cmd->skipChecks)
         assert(valid && (!needData || valid->type != VSH_OT_BOOL));
 
-        if (valid->flags & VSH_OFLAG_REQ)
-            ret = -1;
-    }
+    if (valid && valid->flags & VSH_OFLAG_REQ)
+        ret = -1;
 
     /* See if option is present on command line.  */
     while (candidate) {
@@ -1065,7 +1064,8 @@ vshCommandOptStringReq(vshControl *ctl,
         error = N_("Option argument is empty");
 
     if (error) {
-        vshError(ctl, _("Failed to get option '%s': %s"), name, _(error));
+        if (!cmd->skipChecks)
+            vshError(ctl, _("Failed to get option '%s': %s"), name, _(error));
         return -1;
     }
 
-- 
2.16.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] vshCommandOpt: Do more checking if skipChecks is set
Posted by Martin Kletzander 6 years ago
On Wed, Feb 28, 2018 at 10:20:51AM +0100, Michal Privoznik wrote:
>Currently if cmd->skipChecks is set (done only from completers)
>some basic checks are skipped because we're working over
>partially parsed command. See a26ff63ae4 for more detailed
>explanation. Anyway, the referenced commit was too aggressive in
>disabling checks and effectively returned success even in clear
>case of failure. For instance:
>
>  # domif-getlink --interface <TAB><TAB>
>
>causes virshDomainInterfaceCompleter() to be called, which calls
>virshDomainGetXML() which eventually calls
>vshCommandOptStringReq(.., name = "domain"); The --domain
>argument is required for the command and if not present -1 should
>be returned to tell the caller the argument was not found. Well,
>zero is returned meaning the argument was not found but it's not
>required either.
>
>Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>---
> tools/vsh.c | 20 ++++++++++----------
> 1 file changed, 10 insertions(+), 10 deletions(-)
>

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