1. Patchew
  2. Libvirt
  3. View series

[libvirt] [PATCH] qemu: maintain user alias for video type 'none'

Jonathon Jongsma posted 1 patch 4 years, 7 months ago
Test syntax-check passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/20190905161738.6520-1-jjongsma@redhat.com
src/conf/domain_conf.c                         | 7 +++++--
tests/qemuxml2argvdata/video-none-device.xml   | 1 +
tests/qemuxml2xmloutdata/video-none-device.xml | 1 +
3 files changed, 7 insertions(+), 2 deletions(-)
[libvirt] [PATCH] qemu: maintain user alias for video type 'none'
Posted by Jonathon Jongsma 4 years, 7 months ago 
After parsing a video device with a model type of
VIR_DOMAIN_VIDEO_TYPE_NONE, all device info is cleared (see
virDomainDefPostParseVideo()) in order to avoid formatting any
auto-generated values for the XML. Subsequently, however, an alias is
generated for the video device (e.g. 'video0'), which results in an
alias property being formatted in the XML output anyway. This creates
confusion if the user has explicitly provided an alias for the video
device since the alias will change.

To avoid this, don't clear the user-defined alias for video devices of
type "none".

See https://bugzilla.redhat.com/show_bug.cgi?id=1720612

Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
---
 src/conf/domain_conf.c                         | 7 +++++--
 tests/qemuxml2argvdata/video-none-device.xml   | 1 +
 tests/qemuxml2xmloutdata/video-none-device.xml | 1 +
 3 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 6c429cd593..d11d1fb903 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -5635,11 +5635,14 @@ virDomainDefPostParseVideo(virDomainDefPtr def,
         return 0;
 
     if (def->videos[0]->type == VIR_DOMAIN_VIDEO_TYPE_NONE) {
+        char *alias;
         /* we don't want to format any values we automatically fill in for
-         * videos into the XML, so clear them
-         */
+         * videos into the XML, so clear them, but retain any user-assigned
+         * alias */
+        VIR_STEAL_PTR(alias, def->videos[0]->info.alias);
         virDomainVideoDefClear(def->videos[0]);
         def->videos[0]->type = VIR_DOMAIN_VIDEO_TYPE_NONE;
+        VIR_STEAL_PTR(def->videos[0]->info.alias, alias);
     } else {
         virDomainDeviceDef device = {
             .type = VIR_DOMAIN_DEVICE_VIDEO,
diff --git a/tests/qemuxml2argvdata/video-none-device.xml b/tests/qemuxml2argvdata/video-none-device.xml
index 4b591562b7..c1c60c1d9e 100644
--- a/tests/qemuxml2argvdata/video-none-device.xml
+++ b/tests/qemuxml2argvdata/video-none-device.xml
@@ -31,6 +31,7 @@
     <graphics type='vnc'/>
     <video>
       <model type='none'/>
+      <alias name='ua-user-video-alias'/>
     </video>
     <memballoon model='virtio'>
       <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/>
diff --git a/tests/qemuxml2xmloutdata/video-none-device.xml b/tests/qemuxml2xmloutdata/video-none-device.xml
index 6e76b394fe..82615f5125 100644
--- a/tests/qemuxml2xmloutdata/video-none-device.xml
+++ b/tests/qemuxml2xmloutdata/video-none-device.xml
@@ -34,6 +34,7 @@
     </graphics>
     <video>
       <model type='none'/>
+      <alias name='ua-user-video-alias'/>
     </video>
     <memballoon model='virtio'>
       <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/>
-- 
2.21.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: maintain user alias for video type 'none'
Posted by Erik Skultety 4 years, 7 months ago 
On Thu, Sep 05, 2019 at 11:17:38AM -0500, Jonathon Jongsma wrote:
> After parsing a video device with a model type of
> VIR_DOMAIN_VIDEO_TYPE_NONE, all device info is cleared (see
> virDomainDefPostParseVideo()) in order to avoid formatting any
> auto-generated values for the XML. Subsequently, however, an alias is
> generated for the video device (e.g. 'video0'), which results in an
> alias property being formatted in the XML output anyway. This creates
> confusion if the user has explicitly provided an alias for the video
> device since the alias will change.
>
> To avoid this, don't clear the user-defined alias for video devices of
> type "none".
>
> See https://bugzilla.redhat.com/show_bug.cgi?id=1720612

s/See//   - we had a discussion about using/not using "Resolves: <url>" across
the commit messages; right now, the custom is to mention just the plain URL.

Btw. I had the very same question you posted to the BZ so thanks for asking.
It is a bit unfortunate, that we add a default <video> when there's a
<graphics> element present, because it makes things a bit complicated resulting
in introduction of a "none" device type, but it is what it is and I don't think
ovirt has really an alternative :(.

>
> Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
> ---
>  src/conf/domain_conf.c                         | 7 +++++--
>  tests/qemuxml2argvdata/video-none-device.xml   | 1 +
>  tests/qemuxml2xmloutdata/video-none-device.xml | 1 +
>  3 files changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 6c429cd593..d11d1fb903 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -5635,11 +5635,14 @@ virDomainDefPostParseVideo(virDomainDefPtr def,
>          return 0;
>
>      if (def->videos[0]->type == VIR_DOMAIN_VIDEO_TYPE_NONE) {
> +        char *alias;

I'd put a blank line here to separate the declaration from everything else.

Reviewed-by: Erik Skultety <eskultet@redhat.com>

>          /* we don't want to format any values we automatically fill in for
> -         * videos into the XML, so clear them
> -         */
> +         * videos into the XML, so clear them, but retain any user-assigned
> +         * alias */
> +        VIR_STEAL_PTR(alias, def->videos[0]->info.alias);
>          virDomainVideoDefClear(def->videos[0]);
>          def->videos[0]->type = VIR_DOMAIN_VIDEO_TYPE_NONE;
> +        VIR_STEAL_PTR(def->videos[0]->info.alias, alias);
>      } else {
>          virDomainDeviceDef device = {
>              .type = VIR_DOMAIN_DEVICE_VIDEO,
> diff --git a/tests/qemuxml2argvdata/video-none-device.xml b/tests/qemuxml2argvdata/video-none-device.xml
> index 4b591562b7..c1c60c1d9e 100644
> --- a/tests/qemuxml2argvdata/video-none-device.xml
> +++ b/tests/qemuxml2argvdata/video-none-device.xml
> @@ -31,6 +31,7 @@
>      <graphics type='vnc'/>
>      <video>
>        <model type='none'/>
> +      <alias name='ua-user-video-alias'/>
>      </video>
>      <memballoon model='virtio'>
>        <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/>
> diff --git a/tests/qemuxml2xmloutdata/video-none-device.xml b/tests/qemuxml2xmloutdata/video-none-device.xml
> index 6e76b394fe..82615f5125 100644
> --- a/tests/qemuxml2xmloutdata/video-none-device.xml
> +++ b/tests/qemuxml2xmloutdata/video-none-device.xml
> @@ -34,6 +34,7 @@
>      </graphics>
>      <video>
>        <model type='none'/>
> +      <alias name='ua-user-video-alias'/>
>      </video>
>      <memballoon model='virtio'>
>        <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/>
> --
> 2.21.0
>
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: maintain user alias for video type 'none'
Posted by Daniel P. Berrangé 4 years, 7 months ago 
On Fri, Sep 06, 2019 at 09:58:32AM +0200, Erik Skultety wrote:
> On Thu, Sep 05, 2019 at 11:17:38AM -0500, Jonathon Jongsma wrote:
> > After parsing a video device with a model type of
> > VIR_DOMAIN_VIDEO_TYPE_NONE, all device info is cleared (see
> > virDomainDefPostParseVideo()) in order to avoid formatting any
> > auto-generated values for the XML. Subsequently, however, an alias is
> > generated for the video device (e.g. 'video0'), which results in an
> > alias property being formatted in the XML output anyway. This creates
> > confusion if the user has explicitly provided an alias for the video
> > device since the alias will change.
> >
> > To avoid this, don't clear the user-defined alias for video devices of
> > type "none".
> >
> > See https://bugzilla.redhat.com/show_bug.cgi?id=1720612
> 
> s/See//   - we had a discussion about using/not using "Resolves: <url>" across
> the commit messages; right now, the custom is to mention just the plain URL.

'Resolves:' is just redundant noise - people only do it out of habit because
Red Hat's own internal dev tools for RHEL require that for beurocratic
reasons.

In any case, the most important rule is that the commit message should fully
describe the problem. ie it should be possible to understand the commit
without opening the BZ link at all.


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.