[libvirt] [PATCH 3/5] conf: add virDomainDefAddController()

Laine Stump laine at laine.org
Mon Jan 11 18:29:47 UTC 2016


On 12/09/2015 09:01 AM, John Ferlan wrote:
>
> On 11/19/2015 01:25 PM, Laine Stump wrote:
>> We need a virDomainDefAddController() that doesn't check for an
>> existing controller at the same index (since USB2 controllers must be
>> added in sets of 4 that are all at the same index), so rather than
>> duplicating the code in virDomainDefMaybeAddController(), split it
>> into two functions, in the process eliminating existing duplicated
>> code that loops through the controller list by calling
>> virDomainControllerFind(), which does the same thing).
> As I found while working on a different issue - the "MaybeAdd" and "Add"
> functions were h

You left a thought half-fin

:-)

>> ---
>>   src/conf/domain_conf.c | 56 +++++++++++++++++++++++++++++++++++---------------
>>   1 file changed, 40 insertions(+), 16 deletions(-)
>>
>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>> index 0ac7dbf..ab05e7f 100644
>> --- a/src/conf/domain_conf.c
>> +++ b/src/conf/domain_conf.c
>> @@ -13456,6 +13456,18 @@ virDomainControllerFind(virDomainDefPtr def,
>>   }
>>   
>>   
>> +static int
>> +virDomainControllerUnusedIndex(virDomainDefPtr def, int type)
> How about "FindUnusedIndex" or "GetUnusedIndex" ?

Okay. Done.

>
>
>> +{
>> +    int idx = 0;
>> +
>> +    while (virDomainControllerFind(def, type, idx) >= 0)
>> +        idx++;
>> +
> Something tells me virDomainControllerFind could one day be a bottleneck
> as each time through we start at 0 (ncontrollers) looking for matching
> 'type' && 'idx'... I'm thinking of the recent head banging over the
> network device (?tap*?) lookup algorithm...

The difference is that for the network device we were doing an entire 
netlink message/response cycle for each index that we tried, which was 
*very* slow. So we made a data structure in memory that we could more 
easily scan. In this case we are already working with data in memory 
that we can easily scan, so I think there would be very little to gain.

(note that in the case of tap devices, the kernel will already 
automatically create a unique name for us, so we don't need to try and 
guess as we do for macvtap devices).

>
> One would think we don't have that many different controllers to make a
> difference, but then again did we ever assume the same for the network
> algorithm?

No, I think the people who added macvtap were just trying to make 
something that worked and figured they'd come back and optimize it 
later, but then forgot the 2nd half.


>   Whether creating a hash tree for each type of controller is
> worth the effort would seem to be outside the scope of this set of
> patches, but perhaps something that needs to be considered.

Since everything is just looking through the entries of an array in 
memory comparing integers, I don't see the problem with leaving it like 
this even for a few hundred controllers, and I think we will have fallen 
to the machine overlords before we have to deal with a single virtual 
machine with more controllers than that.

>
> That being said - since it seems we're trying to find an available index
> for a specific type of controller, perhaps it would be better to do
> something like:
>
>      idx = 0;
>
>      for (i = 0; i < def->ncontrollers; i++) {
>          if (def->controllers[i]->type == type) {
>              if (def->controllers[i]->idx != idx)
>                  return idx;
>              idx++;
>          }

You're assuming that the items in the list are in order of index. 
Although there is a function that does that when inserting new elements 
(virDomainControllerInsertPreAlloced()) I don't have complete confidence 
that it is always used to add new controllers. And besides I think the 
counts are low enough that a simple function will do fine (and be easier 
to understand - it took me a few minutes to parse yours and understand 
its assumptions).

(oh, as a matter of fact, virDomainControllerInsertPreAlloced() *isn't* 
always called - virDomainDefMaybeAddController() didn't use it, and 
after this patch virDomainControllerDefAddController() doesn't (because 
it just used the existing code in the other function).


>      }
>      return idx;
>
>
>
>> +    return idx;
>> +}
>> +
>> +
>>   const char *
>>   virDomainControllerAliasFind(virDomainDefPtr def,
>>                                int type, int idx)
>> @@ -14375,33 +14387,45 @@ virDomainEmulatorPinDefParseXML(xmlNodePtr node)
>>   }
>>   
>>   
>> -int
>> -virDomainDefMaybeAddController(virDomainDefPtr def,
>> -                               int type,
>> -                               int idx,
>> -                               int model)
>> +static virDomainControllerDefPtr
>> +virDomainDefAddController(virDomainDefPtr def, int type, int idx, int model)
>>   {
>> -    size_t i;
>>       virDomainControllerDefPtr cont;
>>   
>> -    for (i = 0; i < def->ncontrollers; i++) {
>> -        if (def->controllers[i]->type == type &&
>> -            def->controllers[i]->idx == idx)
>> -            return 0;
>> -    }
>> -
>>       if (!(cont = virDomainControllerDefNew(type)))
>> -        return -1;
>> +        return NULL;
>> +
>> +    if (idx < 0)
>> +        idx = virDomainControllerUnusedIndex(def, type);
>>   
>>       cont->idx = idx;
>>       cont->model = model;
>>   
>> -    if (VIR_APPEND_ELEMENT(def->controllers, def->ncontrollers, cont) < 0) {
>> +    if (VIR_APPEND_ELEMENT_COPY(def->controllers, def->ncontrollers, cont) < 0) {
>>           VIR_FREE(cont);
>> -        return -1;
>> +        return NULL;
>>       }
>>   
>> -    return 1;
>> +    return cont;
>> +}
>> +
>> +
>> +int
>> +virDomainDefMaybeAddController(virDomainDefPtr def,
>> +                               int type,
>> +                               int idx,
>> +                               int model)
>> +{
>> +    /* skip if a specific index was given and it is already
>> +     * in use for that type of controller
>> +     */
>> +    if (idx >= 0 && virDomainControllerFind(def, type, idx) >= 0)
>> +        return 0;
>> +
>> +    if (virDomainDefAddController(def, type, idx, model))
>> +        return 1;
>> +    else
>> +        return -1;
> The "else" isn't necessarily necessary ;-)

Yeah. I'll fix that.

>
> ACK - I would like to see the function name change only to indicate that
> it's an action Find/Get.  Whether you adjust the find algorithm depends
> on whether you'd like to revisit this code some day...

Okay. I changed the name and eliminated the extra superfluous else.




More information about the libvir-list mailing list