2:25 a.m.
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(a)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;
>