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@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@redhat.com>