Re: [libvirt PATCH v4 15/31] qemu: log error output from nbdkit
On Fri, Jan 20, 2023 at 16:03:09 -0600, Jonathon Jongsma wrote:
log stderr and stdout from nbdkit into its own log so that
nbdkit-related issues can be debugged more easily.
Signed-off-by: Jonathon Jongsma <jjongsma(a)redhat.com>
---
src/qemu/qemu_nbdkit.c | 28 ++++++++++++++++++++--------
1 file changed, 20 insertions(+), 8 deletions(-)
diff --git a/src/qemu/qemu_nbdkit.c b/src/qemu/qemu_nbdkit.c
index 34462f784a..7a7b9a6a54 100644
--- a/src/qemu/qemu_nbdkit.c
+++ b/src/qemu/qemu_nbdkit.c
@@ -848,15 +848,24 @@ qemuNbdkitProcessStart(qemuNbdkitProcess *proc,
int rc;
int exitstatus = 0;
int cmdret = 0;
- VIR_AUTOCLOSE errfd = -1;
virTimeBackOffVar timebackoff;
bool socketCreated = false;
+ g_autofree char *basename = g_strdup_printf("%s-nbdkit-%i",
vm->def->name, proc->source->id);
+ int logfd = -1;
+ g_autoptr(qemuLogContext) logContext = NULL;
+ g_autofree char *errmsg = NULL;
+ g_autoptr(virURI) uri = NULL;
+ g_autofree char *uristring = NULL;
if (!(cmd = qemuNbdkitProcessBuildCommand(proc)))
return -1;
+ logContext = qemuLogContextNew(driver, vm, basename);
This can return NULL ...
+ logfd = qemuLogContextGetWriteFD(logContext);
and this will dereference it unconditionally.
+
VIR_DEBUG("starting nbdkit process for %s",
proc->source->nodestorage);
- virCommandSetErrorFD(cmd, &errfd);
+ virCommandSetErrorFD(cmd, &logfd);
+ virCommandSetOutputFD(cmd, &logfd);
virCommandSetPidFile(cmd, proc->pidfile);
if (qemuExtDeviceLogCommand(driver, vm, cmd, "nbdkit") < 0)
@@ -900,12 +909,15 @@ qemuNbdkitProcessStart(qemuNbdkitProcess *proc,
return 0;
error:
- if (errfd > 0) {
- g_autofree char *errbuf = g_new0(char, 1024);
- if (read(errfd, errbuf, 1024) > 0)
- virReportError(VIR_ERR_OPERATION_FAILED,
- _("nbdkit failed to start and reported: %s"),
errbuf);
- }
+ if (qemuLogContextReadFiltered(logContext, &errmsg, 1024) < 0)
+ VIR_WARN("Unable to read from nbdkit log");
+
+ uri = qemuBlockStorageSourceGetURI(proc->source);
This can return NULL, and also was not really designed to be used for
the SSH protocol although works here ...
+ uristring = virURIFormat(uri);
... and this will dereference it unconditionally.
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Failed to connect to nbdkit for '%s': %s"),
+ uristring, errmsg);
The first problem has an obvious fix, but it's a bit less obvious for
the latter. I suggest using NULLSTR(uristring) and simply making the
URI->string conversion based on 'uri' being non-null as it's an unlikely
problem.
With the proper fixes:
Reviewed-by: Peter Krempa <pkrempa(a)redhat.com>