Re: [libvirt] [PATCH 08/18] vshCommandOpt: Allow caller avoiding assert()

On Thu, Jan 11, 2018 at 06:02:58AM -0500, John Ferlan wrote: > > > On 01/11/2018 05:50 AM, Martin Kletzander wrote: >> On Tue, Jan 02, 2018 at 06:12:01PM +0100, Michal Privoznik wrote: >>> In the future, completer callbacks will receive partially parsed >>> command (and thus possibly incomplete). However, we still want >>> them to use command options fetching APIs we already have (e.g. >>> vshCommandOpt*()) and at the same time don't report any errors >>> (nor call any asserts). >>> >>> Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; >>> --- >>> tools/vsh.c | 7 ++++--- >>> tools/vsh.h | 3 ++- >>> 2 files changed, 6 insertions(+), 4 deletions(-) >>> >>> diff --git a/tools/vsh.c b/tools/vsh.c >>> index ebc8d9cb1..d27acb95b 100644 >>> --- a/tools/vsh.c >>> +++ b/tools/vsh.c >>> @@ -815,8 +815,8 @@ vshCommandFree(vshCmd *cmd) >>>  * 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 >>> - * issued if a value is returned. >>> + * valid (which indicates a programming error) unless cmd->skipChecks >>> + * is set. No error messages are issued if a value is returned. >>>  */ >>> static int >>> vshCommandOpt(const vshCmd *cmd, const char *name, vshCmdOpt **opt, >>> @@ -829,7 +829,8 @@ vshCommandOpt(const vshCmd *cmd, const char *name, >>> vshCmdOpt **opt, >>>     /* See if option is valid and/or required.  */ >>>     *opt = NULL; >>>     while (valid) { >>> -        assert(valid->name); >>> +        if (!cmd->skipChecks) >>> +            assert(valid->name); >> >> This can segfault when cmd->skipChecks == False && valid->name == NULL, >> which is what the assert() guarded before. >> >> So either STREQ_NULLABLE or another if. >> > > Hmmm... Also see: > > https://www.redhat.com/archives/libvir-list/2017-December/msg00605.html > > it's related somewhat... > I don't see how, this is all wrapped in `while (valid)`