Re: [libvirt] [PATCH v2 3/3] iohelper: Don't include newlines in error messages
On 2/19/19 8:44 AM, Andrea Bolognani wrote:
On Mon, 2019-02-18 at 11:05 -0500, John Ferlan wrote:
> On 2/13/19 7:04 AM, Andrea Bolognani wrote:
>> The newline was pretty arbitrary, and we're better off
>> without it.
>
> I'm mostly ambivalent about this one; however, since
> virGetLastErrorMessage could return a string without a "\n", then
> perhaps it's best to keep the \n since it really doesn't hurt.
Without this patch, the user will end up seeing
$ sudo virsh dump guest /small/guest.dump --memory-only
error: Failed to core dump domain guest to /small/guest.dump
error: operation failed: /usr/libexec/libvirt_iohelper: failure with /small/guest.dump
: Unable to write /small/guest.dump: No space left on device
instead of the more reasonable
$ sudo virsh dump guest /small/guest.dump --memory-only
error: Failed to core dump domain guest to /small/guest.dump
error: operation failed: /usr/libexec/libvirt_iohelper: failure with /small/guest.dump:
Unable to write /small/guest.dump: No space left on device
Now, neither is optimal and the way libvirt_iohelper formats its
error messages should be tweaked further, but at least in the latter
case the error is not split randomly with the second line starting
with a colon, which is an improvement in my book.
I was also unable to find other examples of messages passed to
virReportError(), which is what will ultimately happen to this
output, containing newlines. To be fair, I have not really spent a
lot of time looking for them either :)
I didn't dig too much either, but I was perhaps overthinking the other
end of the message. I went straight to virGetLastErrorMessage and noted
it "could" return "no error" or "unknown error" without the
'\n' at the
end which IIUC would be generated for virReportError (or err->message)
values.
Anyway, see commit b29e08db... I think a case could be made in the
commit message ;-) that prior to that commit the '\n' was (properly) at
the end, but with that commit message the '\n' was perhaps put in the
wrong place.
So maybe this changes to move the '\n' to after the formatted message.
Of course that means perhaps an extra blank line for most outputs.
In the long run, it's a blank line adjustment, so consider this
Reviewed-by: John Ferlan <jferlan(a)redhat.com>
for me too (I see Dan responded in the mean time).
John