[libvirt] [PATCH v4 14/20] qemu: add vhost-user-gpu helper unit

[Ján Tomko]

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 at redhat.com> wrote:
>>
>> On 9/13/19 8:50 AM, marcandre.lureau at redhat.com wrote:
>> > From: Marc-André Lureau <marcandre.lureau at redhat.com>
>> >
>> > 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 at redhat.com>
>> > ---
>> >   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 object
>
>It 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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20190923/68f35672/attachment-0001.sig>