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?