[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



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


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