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:
pgpewRt9V8Aji.pgp
Description: PGP signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list