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@redhat.com> for me too (I see Dan responded in the mean time). John