[libvirt] [PATCH v2 2/4] conf: add optional attribte primary to video <model> element

[Laine Stump]

On 12/11/2012 01:11 PM, Eric Blake wrote:
> On 12/11/2012 07:14 AM, Guannan Ren wrote:
>> If there are multiple video devices
>> primary = 'yes' marks this video device as the primary one.
>> The rest are secondary video devices. No more than one could be
>> mark as primary. If none of them has primary attribute, the first
>> one will be the primary by default like what it was.
>> The reason of this changing is that for qemu, only one primary video
>> device is permitted which can be of any type. For secondary video
>> devices, only qxl is allowd. Primary attribute removes the restriction
>> that the first have to be the primary one.
>>
>> We always put the primary video device into the first position of
>> video device structure array after parsing.
>> ---
>>  src/conf/domain_conf.c | 25 +++++++++++++++++++++++++
>>  src/conf/domain_conf.h |  1 +
>>  2 files changed, 26 insertions(+)
> Incomplete.  I can't approve this without a change to
> docs/schemas/domaincommon.rng to parse the new attribute, and to
> docs/formatdomain.html.in to document it.
>
>> @@ -7289,6 +7290,11 @@ virDomainVideoDefParseXML(const xmlNodePtr node,
>>                  type = virXMLPropString(cur, "type");
>>                  vram = virXMLPropString(cur, "vram");
>>                  heads = virXMLPropString(cur, "heads");
>> +
>> +                if ((primary = virXMLPropString(cur, "primary"))!= NULL)
> Space before !=.
>
>> @@ -9961,6 +9968,22 @@ static virDomainDefPtr virDomainDefParseXML(virCapsPtr caps,
>>                                                                 flags);
>>          if (!video)
>>              goto error;
>> +
>> +        if (!primaryVideo && video->primary) {
>> +            if (def->nvideos != 0)
>> +                memmove(def->videos + 1,
>> +                        def->videos,
>> +                        (sizeof(def->videos[0]) * def->nvideos));
>> +
>> +            def->videos[0] = video;
>> +            def->nvideos++;
>> +            primaryVideo = true;
> This would be a good candidate to use Laine's new VIR_INSERT_ELEMENT macro.

Heh. Yep. Even in my patches that I didn't push, I hadn't done the
substitution here because it was only appending and only preallocated
memory (so there was no real gain). But since you're optionally
inserting at the beginning, you could do this:

            size_t ii = def->nvideos;
            if (video->primary) {
                if (primaryVideo) {

                    virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
                                   _("Only one primary video device is supported"));
                    goto error;
                }

                ii = 0;
                primaryVideo = true;
            }
            VIR_INSERT_ELEMENT_INPLACE(def->videos, ii, def->nvideos,
video);

(then you don't need the "def->videos[def->nvideos++] = video" that
follows the chunk from your original patch)