On 02/21/2013 04:32 PM, Christophe Fergeau wrote:
Hey Martin,
Sorry, took me a while to get back to this patch,
No problem, I had the same issue :)
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 haven't noticed the other if()s and we have no consistent style for this, so this doesn't matter, keep it this way.
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?
I was just thinking out loud how to deal with this, but it's related to the devices. There is a way how to detect those revisions etc., but those are more cumbersome than helpful, so don't mind me talking about that :) If somebody will have something against it, they will say so in the v2 (I'll be mostly out for a few days from now). Have a nice weekend, Martin