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