[libvirt] [PATCHv2 1/4] conf: Add helpers to insert/remove/find shmem devices in domain def

[lhuang]

On 12/10/2015 08:50 AM, John Ferlan wrote:
>
> On 11/26/2015 04:06 AM, Luyao Huang wrote:
>> The helpers will be useful when implementing hotplug and coldplug of
>> shared memory devices.
>>
>> Signed-off-by: Luyao Huang <lhuang at redhat.com>
>> ---
>>   src/conf/domain_conf.c   | 66 ++++++++++++++++++++++++++++++++++++++++++++++++
>>   src/conf/domain_conf.h   |  7 +++++
>>   src/libvirt_private.syms |  3 +++
>>   3 files changed, 76 insertions(+)
>>
> Although there is a v1, it doesn't seem patches 6-10 of that series
> (e.g. these patches) were reviewed.  Is that correct?  Just want to make
> sure I'm not missing something from someone else's review...
>
> anyway...

here is the review of patch 10:

https://www.redhat.com/archives/libvir-list/2015-July/msg00338.html

patch 6:

https://www.redhat.com/archives/libvir-list/2015-July/msg00319.html

And patch 7-9 were not reviewed, i posted a new version since i 
introduced a new parameter in virDomainShmemFind() and removed audit 
part (I remember Martin will help finish that part, but i am sorry that 
i forgot mention this in some place)

>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>> index 616bf80..cc2a767 100644
>> --- a/src/conf/domain_conf.c
>> +++ b/src/conf/domain_conf.c
>> @@ -13950,6 +13950,72 @@ virDomainMemoryRemove(virDomainDefPtr def,
>>   }
>>   
>>   
>> +int
>> +virDomainShmemInsert(virDomainDefPtr def,
>> +                     virDomainShmemDefPtr shmem)
> I see you've followed the virDomainHostdevInsert model...
>
>> +{
>> +    return VIR_APPEND_ELEMENT(def->shmems, def->nshmems, shmem);
>> +}
>> +
>> +
> Would be nice to have an intro comment explaining the params (for other
> two as well), but this one is a bit more "interesting" because of arg3.
>   This includes adding return value.

Okay, i will add them in next version.

>> +ssize_t
>> +virDomainShmemFind(virDomainDefPtr def,
> More or less follows virDomainHostdevFind, but could also be more
> explicit using "FindIndex" or "FindIdx"... Not that important.
>
>> +                   virDomainShmemDefPtr shmem,
>> +                   bool fullmatch)
> I think this is more "match name only".  It could also be an "unsigned
> int flags" where the flags is an enum that could dictate the level of
> match required by the caller (perhaps overkill for what is necessary,
> but flags just seems to be a better option in my opinion.  A passed
> value of 0 for flags would indicate to match everything

interesting idea, i will have a try with your option, you will see it in 
next version :)

> Beyond that, following virDomainHostdevFind - you could perhaps return
> an int. We know size_t will be 0 to def->nshmems...

got it.

>> +{
>> +    size_t i;
>> +
>> +    for (i = 0; i < def->nshmems; i++) {
>> +         virDomainShmemDefPtr tmpshmem = def->shmems[i];
>> +
>> +         if (STREQ(shmem->name, tmpshmem->name)) {
>> +             if (!fullmatch)
>> +                 return i;
>> +         } else {
>> +             continue;
>> +         }
> Perhaps cleaner as:
>
>      if (STRNEQ(shmem->name, tmpshmem->name))
>          continue;
>
>      if (!fullmatch)
>          return i;
>
> You could also use matchnameonly or (flags &
> VIR_DOMAIN_SHMEM_NAME_MATCH_ONLY)

Okay

>> +
>> +         if (shmem->size != tmpshmem->size)
>> +             continue;
>> +
> Eventually someone could add other flags such as
>
>   -> match name and size only...
>   -> match name, size, and server
>   -> match name, size, server, and msi
>   -> match name, size, server, msi, and info/address
>

I see.

>> +         if (shmem->server.enabled != tmpshmem->server.enabled ||
>> +             (shmem->server.enabled &&
>> +              STRNEQ_NULLABLE(shmem->server.chr.data.nix.path,
>> +                              tmpshmem->server.chr.data.nix.path)))
>> +             continue;
>> +
>> +         if (shmem->msi.enabled != tmpshmem->msi.enabled ||
>> +             (shmem->msi.enabled &&
>> +              (shmem->msi.vectors != tmpshmem->msi.vectors ||
>> +               shmem->msi.ioeventfd != tmpshmem->msi.ioeventfd)))
>> +             continue;
>> +
>> +        if (shmem->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE &&
>> +            !virDomainDeviceInfoAddressIsEqual(&shmem->info, &tmpshmem->info))
>> +            continue;
> Matching address is such a touchy thing, I know why it's here to be
> complete, but since the remove device doesn't have to add it to the XML
> passed in, is there a need?   I guess we can leave it for now unless
> someone else offers up a different opinion.
>

Indeed, i will remove this check in next version.

>> +
>> +        break;
> Why not just return i; here?

You are right, i will fix this and ...

>> +    }
>> +
>> +    if (i < def->nshmems)
>> +        return i;
> Removing need for this check

... this in next version

Thanks a lot for your reviews and suggestions.

Luyao

>> +
>> +    return -1;
>> +}
>> +
>> +
>> +virDomainShmemDefPtr
>> +virDomainShmemRemove(virDomainDefPtr def,
>> +                     size_t idx)
>> +{
>> +    virDomainShmemDefPtr ret = def->shmems[idx];
>> +
>> +    VIR_DELETE_ELEMENT(def->shmems, idx, def->nshmems);
>> +
>> +    return ret;
>> +}
>> +
>> +
>>   char *
>>   virDomainDefGetDefaultEmulator(virDomainDefPtr def,
>>                                  virCapsPtr caps)
>> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
>> index 8d43ee6..ee76e3f 100644
>> --- a/src/conf/domain_conf.h
>> +++ b/src/conf/domain_conf.h
>> @@ -3012,6 +3012,13 @@ int virDomainMemoryFindInactiveByDef(virDomainDefPtr def,
>>                                        virDomainMemoryDefPtr mem)
>>       ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK;
>>   
>> +int virDomainShmemInsert(virDomainDefPtr def, virDomainShmemDefPtr shmem)
>> +    ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK;
>> +ssize_t virDomainShmemFind(virDomainDefPtr def, virDomainShmemDefPtr shmem, bool fullmatch)
>> +    ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK;
>> +virDomainShmemDefPtr virDomainShmemRemove(virDomainDefPtr def, size_t idx)
>> +    ATTRIBUTE_NONNULL(1);
>> +
>>   VIR_ENUM_DECL(virDomainTaint)
>>   VIR_ENUM_DECL(virDomainVirt)
>>   VIR_ENUM_DECL(virDomainBoot)
>> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
>> index 7e60d87..88c2c53 100644
>> --- a/src/libvirt_private.syms
>> +++ b/src/libvirt_private.syms
>> @@ -451,6 +451,9 @@ virDomainSaveStatus;
>>   virDomainSaveXML;
>>   virDomainSeclabelTypeFromString;
>>   virDomainSeclabelTypeToString;
>> +virDomainShmemFind;
>> +virDomainShmemInsert;
>> +virDomainShmemRemove;
>>   virDomainShutdownReasonTypeFromString;
>>   virDomainShutdownReasonTypeToString;
>>   virDomainShutoffReasonTypeFromString;
>>