[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH] Add support for qxl.revision in domain XML



Hey Martin,

Sorry, took me a while to get back to this patch,

On Thu, Feb 14, 2013 at 05:54:02PM +0100, Martin Kletzander wrote:
> On 02/04/2013 04:16 PM, Christophe Fergeau wrote:
> > ---
> >  docs/formatdomain.html.in                               |  5 ++++-
> >  docs/schemas/domaincommon.rng                           |  5 +++++
> >  src/conf/domain_conf.c                                  | 17 +++++++++++++++++
> >  src/conf/domain_conf.h                                  |  1 +
> >  src/qemu/qemu_command.c                                 |  8 ++++++++
> >  .../qemuxml2argv-graphics-spice-compression.args        |  3 ++-
> >  .../qemuxml2argv-graphics-spice-compression.xml         |  4 ++--
> >  .../qemuxml2argv-graphics-spice-qxl-vga.args            |  3 ++-
> >  .../qemuxml2argv-graphics-spice-qxl-vga.xml             |  4 ++--
> >  tests/qemuxml2argvdata/qemuxml2argv-graphics-spice.args |  3 ++-
> >  tests/qemuxml2argvdata/qemuxml2argv-graphics-spice.xml  |  4 ++--
> >  11 files changed, 47 insertions(+), 10 deletions(-)
> > 
> > diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> > index 7ad8aea..4b269c8 100644
> > --- a/docs/formatdomain.html.in
> > +++ b/docs/formatdomain.html.in
> > @@ -3569,7 +3569,10 @@ qemu-kvm -net nic,model=? /dev/null
> >          attribute <code>ram</code> (<span class="since">since
> >          1.0.2</span>) is allowed for "qxl" type only and specifies
> >          the size of the primary bar, while <code>vram</code> specifies the
> > -        secondary bar size.  If "ram" is not supplied a default value is used.
> > +        secondary bar size.  If "ram" or "vram" are not supplied a default
> 
> Good fix, but it should in a be separate patch.

Yes, split, I'll push this doc change under the trivial rule (unless
there's a freeze going on).

> 
> > +        value is used. The optional attribute <code>revision</code> (<span
> > +        class="since">since 1.0.3</span>) specifies the revision of
> > +        the QXL device, newer revisions provides more functionality.
> 
> s/provides/provide/ if I'm not mistaken

Yes, I agree, changed.

> 
> >        </dd>
> >  
> >        <dt><code>model</code></dt>
> > diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
> > index 049f232..fc78e2d 100644
> > --- a/docs/schemas/domaincommon.rng
> > +++ b/docs/schemas/domaincommon.rng
> > @@ -2280,6 +2280,11 @@
> >                    <ref name="unsignedInt"/>
> >                  </attribute>
> >                </optional>
> > +              <optional>
> > +                <attribute name="revision">
> > +                  <ref name="unsignedInt"/>
> 
> This should be > 0 according to my experiments with qemu, but I believe
> we don't deal with such nuances in the RNG scheme.

I indeed don't know how to do that, and I couldn't find attributes doing it
so it's going to stay as is for now ;)

> 
> > +                </attribute>
> > +              </optional>
> >              </group>
> >            </choice>
> >            <optional>
> > diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> > index 5782abb..83be711 100644
> > --- a/src/conf/domain_conf.c
> > +++ b/src/conf/domain_conf.c
> > @@ -7391,6 +7391,7 @@ virDomainVideoDefParseXML(const xmlNodePtr node,
> >      char *vram = NULL;
> >      char *ram = NULL;
> >      char *primary = NULL;
> > +    char *revision = NULL;
> >  
> >      if (VIR_ALLOC(def) < 0) {
> >          virReportOOMError();
> > @@ -7406,6 +7407,7 @@ virDomainVideoDefParseXML(const xmlNodePtr node,
> >                  ram = virXMLPropString(cur, "ram");
> >                  vram = virXMLPropString(cur, "vram");
> >                  heads = virXMLPropString(cur, "heads");
> > +                revision = virXMLPropString(cur, "revision");
> 
> You're leaking the revision string in here.

Oops, thanks! Bad news is that there are already other leaks there in error
paths, I'll send patches.

> > diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> > index 9a9e0b1..81925b1 100644
> > --- a/src/conf/domain_conf.h
> > +++ b/src/conf/domain_conf.h
> > @@ -1160,6 +1160,7 @@ struct _virDomainVideoDef {
> >      unsigned int ram;  /* kibibytes (multiples of 1024) */
> >      unsigned int vram; /* kibibytes (multiples of 1024) */
> >      unsigned int heads;
> > +    unsigned int revision;
> >      bool primary;
> >      virDomainVideoAccelDefPtr accel;
> >      virDomainDeviceInfo info;
> > diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> > index f6273c1..e45c808 100644
> > --- a/src/qemu/qemu_command.c
> > +++ b/src/qemu/qemu_command.c
> > @@ -3598,6 +3598,9 @@ qemuBuildDeviceVideoStr(virDomainVideoDefPtr video,
> >  
> >          /* QEMU accepts bytes for vram_size. */
> >          virBufferAsprintf(&buf, ",vram_size=%u", video->vram * 1024);
> > +
> > +        if (video->revision != 0)
> 
> you can drop the '!= 0' in here, but that's unimportant.

I prefer the more explicit version (with != 0), I've kept it this way as
there are other similar if () in the same file.

I'm a bit lost by your conversation with Alon, do you expect more work to
be done to get the supported revisions out of QEMU, or will a v2 fixing
what you pointed out in this email be enough for now?

Thanks,

Christophe

Attachment: pgpPzmj3ePa6f.pgp
Description: PGP signature


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]