Re: [libvirt] [PATCH] qemu: add spice opengl support

> I don't think we want to merge this before a QEMU version that > supports this attribute has been released. Still, it's good to get > ready ahead of time :)   That would delay the support for other projects too (virt-manager/boxes etc). I don't know what rules applies, but similarly we added virtio-gpu support in Nov, a month before the qemu 2.5 release.   This gl=yes argument has been in the work for over a year (in development branches), and it is simple I don't think it will change.

> > @@ -4991,6 +4991,12 @@ qemu-kvm -net nic,model=? /dev/null > >                0.8.8</span>); and <code>usbredir</code> > >                (<span class="since">since 0.9.12</span>). > >              </p> > > +            <p> > > +              Spice may provide accelerated server-side rendering with > > +              OpenGL. You can enable or disable OpenGL support explicitly with > > +              the <code>gl</code> attribute. > > +              (<span class="since">since 1.3.2</span>). > > +            </p> >  > I think this should be a sub-element rather than an attribute, something > like   Any guideline on choosing attribute or sub elements here?

> > @@ -6047,6 +6047,17 @@ qemuBuildGraphicsSPICECommandLine(virQEMUDriverConfigPtr cfg, > >          } > >      } > >  > > +    if (graphics->data.spice.gl) { > > +        if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_SPICE_GL)) { > > +            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", > > +                           _("This QEMU doesn't support spice OpenGL")); > > +            goto error; > > +        } > > + > > +        virBufferAsprintf(&opt, ",gl=%s", > > +                          virTristateSwitchTypeToString(graphics->data.spice.gl)); >  > graphics->data.spice.gl is a virTristateBool, yet you're using > virTristateSwitchTypeToString() on it. >  > I know you have "yes" / "no" in the XML but need "on" / "off" for the > QEMU attribute, and I know that virTristateBool and virTristateSwitch > are basically the same thing, but this still looks weird. >  > I'd rather use something like >  >   switch (graphics->data.spice.gl) { >       case VIR_TRISTATE_BOOL_TRUE: >           virBufferAsprintf(&opt, ",gl=on"); >           break; >       case VIR_TRISTATE_BOOL_FALSE: >           virBufferAsprintf(&opt, ",gl=off"); >           break; >       default: >           break; >   } >  > but other people probably feel differently :)   I understand your concern, but having that switch seems worse to me. Maybe we should make it statically explicit that VIR_TRISTATE_BOOL_YES == VIR_TRISTATE_SWITCH_ON..