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@xxxxxxxxxx>
> ---
> 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@xxxxxxxxxx>
