[libvirt] [PATCH v2 03/16] domain: add rendernode attribute on <accel>

[Ján Tomko]

On Fri, Aug 23, 2019 at 01:04:53PM -0400, Cole Robinson wrote:
>On 8/23/19 12:21 PM, Cole Robinson wrote:
>> From: Marc-André Lureau <marcandre.lureau at redhat.com>
>>
>> vhost-user-gpu helper may accept --render-node option to specify on
>> which GPU should the renderning be done.
>>
>
>What does it do if the user doesn't pass one? Pick one itself, or just
>not use one somehow?
>
>If it picks one, then we may need to treat this like we treat other
>rendernode instances and autoallocate one if the user doesn't specify,
>otherwise we won't be able to add the path to the cgroup for example, or
>selinux label it if necessary. I'm not sure if that actually applies in
>this case, but it's worth considering.
>

Even if we'll chose one automatically, we should at least output it in
live XML.

>> (by comparison <graphics> rendernode is the target/display rendering)
>>
>> Signed-off-by: Marc-André Lureau <marcandre.lureau at redhat.com>
>> Signed-off-by: Cole Robinson <crobinso at redhat.com>
>
>Also I see now that I accidentally signed off all these patches, that
>was not intentional. Please strip those from v3
>

Per https://libvirt.org/governance.html#codeofconduct we need your
intentional agreement with the Developer Certificate of Origin:
https://developercertificate.org/ to be able to include your changes.

I'd be okay from stripping it from the patches you did not actually
change, but it should stay on those where you actually changed
something.

Also, we do not have that process documented, but the kernel practice
(and what I've been doing) seems to be that the maintainer merging the
patch also adds a sign-off to provide that chain of certificates.

Thirdly, I see some patches committed to libvirt by Marc-André but
no mention in AUTHORS.in history - maybe it's time to get his commit
access restored and documented?

>> ---
>>  docs/formatdomain.html.in     |  5 +++++
>>  docs/schemas/domaincommon.rng |  5 +++++
>>  src/conf/domain_conf.c        | 18 +++++++++++++++++-
>>  src/conf/domain_conf.h        |  1 +
>>  4 files changed, 28 insertions(+), 1 deletion(-)
>>

>> @@ -15474,6 +15481,11 @@ virDomainVideoDefParseXML(virDomainXMLOptionPtr xmlopt,
>>              goto error;
>>          }
>>      }
>> +    if (!def->vhostuser && def->accel && def->accel->rendernode) {
>> +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>> +                       _("unsupported rendernode accel attribute without 'vhost-user'"));
>> +        goto error;
>> +    }
>>
>
>This function doesn't represent best practices, but this style of check
>should be moved to virDomainVideoDefValidate IMO
>

Yes.

Jano

>Thanks,
>Cole
>
>--
>libvir-list mailing list
>libvir-list at redhat.com
>https://www.redhat.com/mailman/listinfo/libvir-list
-------------- 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/20190827/85fcd570/attachment-0001.sig>