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(a)redhat.com>
>> - 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.