[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