On Mon, Sep 23, 2019 at 02:16:37PM +0400, Marc-André Lureau wrote:
Hi On Sat, Sep 21, 2019 at 1:05 AM Cole Robinson <crobinso@xxxxxxxxxx> wrote:On 9/13/19 8:50 AM, marcandre.lureau@xxxxxxxxxx wrote: > From: Marc-André Lureau <marcandre.lureau@xxxxxxxxxx> > > Similar to the qemu_tpm.c, add a unit with a few functions to > start/stop and setup the cgroup of the external vhost-user-gpu > process. See function documentation. > > The vhost-user connection fd is set on qemuDomainVideoPrivate struct. > > Signed-off-by: Marc-André Lureau <marcandre.lureau@xxxxxxxxxx> > --- > src/qemu/Makefile.inc.am | 2 + > src/qemu/qemu_domain.c | 5 +- > src/qemu/qemu_domain.h | 2 +- > src/qemu/qemu_vhost_user_gpu.c | 276 +++++++++++++++++++++++++++++++++ > src/qemu/qemu_vhost_user_gpu.h | 49 ++++++ > 5 files changed, 332 insertions(+), 2 deletions(-) > create mode 100644 src/qemu/qemu_vhost_user_gpu.c > create mode 100644 src/qemu/qemu_vhost_user_gpu.h >
[...]
> + virCommandClearCaps(cmd); > + virCommandSetPidFile(cmd, pidfile); > + virCommandDaemonize(cmd); > + > + if (qemuExtDeviceLogCommand(driver, vm, cmd, "vhost-user-gpu") < 0) > + goto error; > + Most, possibly all, of these 'goto error;' usages are overwriting detailed error messages with 'vhost-user-gpu failed to start'. So this needs to be reworked. You can use virGetLastErrorMessage if you wanted to prepend the vhost-user specific bit to the error. Maybe break up this function into chunks, like one to build the Command objectIt doesn't "overwrite" the detailed error, it merely adds another virReportError(). Or am I missing something?
virReportError does two things: * logs an error into the configured log * sets the thread-local error variable The error variable can only store one error and this is what will get propagated to the API users. All previous errors logged via virReportError can only be found in the log. So ideally [0] one error path would only set one error. And vice-versa, once you log an error via virReportError, resetting it via virResetLastError will not unlog it, only reset the local error pointer. This function is intended to be used on public APIs entry points to make sure we don't have a leftover error from previous APIs. Jano
I'll leave that for now.
[0] Ideally to match this design. It might be sensible to report more than one error to the API users, but we do not have the infrastructure for that.
Attachment:
signature.asc
Description: PGP signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
