On Tue, Mar 12, 2019 at 06:40:42 -0500, Eric Blake wrote:
> On 3/12/19 2:50 AM, Erik Skultety wrote:
> > On Mon, Mar 11, 2019 at 09:18:44PM -0500, Eric Blake wrote:
> >> In local testing, I accidentally introduced a self-test failure,
> >> and spent way too much time debugging it. Make sure the testsuite
> >> log includes some hint as to why command option validation failed.
> >>
> >> Signed-off-by: Eric Blake <eblake@xxxxxxxxxx>
>
> >> - if (opt->flags || !opt->help)
> >> + if (opt->flags || !opt->help) {
> >> + vshError(ctl, _("command '%s' has incorrect alias option"),
> >> + cmd->name);
> >> return -1; /* alias options are tracked by the original name */
> >> + }
> >> if ((p = strchr(name, '=')) &&
> >> - VIR_STRNDUP(name, name, p - name) < 0)
> >> + VIR_STRNDUP(name, name, p - name) < 0) {
> >> + vshError(ctl, _("allocation failure while checking command '%s'"),
> >> + cmd->name);
> >
> > I think ^this one can be dropped, if there was an allocation failure, we have
> > much bigger problems and it's likely it will fail again which vshError will
> > transitively try to do if you look at vshOutputLogFile for example.
>
> Agreed. We can't use assert() in libvirt.so, but virsh has other places
> where it fits, so I'll switch this one to assert.
assert() is for debug-builds only, isn't it? Did you mean abort() ?
Still the construction proposed in the patch looks much better than
either of those.
Also for local output the printing is (partially) done into a file
descriptor, so you'll at least get the string 'error' printed.
Note also that virsh uses exit(EXIT_FAILURE) on allocation failure, so
neither abort nor assert should be used.
I think the appropriate solution is vshErrorOOM though, which by the way
is called also if the allocation in vshError fails.
Attachment:
signature.asc
Description: PGP signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
