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

[John Ferlan]

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...

> 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.

> +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

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

> +{
> +    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)

> +
> +         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


> +         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.

> +
> +        break;

Why not just return i; here?

> +    }
> +
> +    if (i < def->nshmems)
> +        return i;

Removing need for this check
> +
> +    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;
>