On Wed, Nov 02, 2016 at 10:22:35AM +0100, Jiri Denemark wrote:
> Let's keep all run time validation of cached QEMU capabilities in
> virQEMUCapsIsValid and call it whenever we access the cache.
> virQEMUCapsInitCached should keep only the checks which do not make
> sense once the cache is loaded in memory.
>
> Signed-off-by: Jiri Denemark <jdenemar@xxxxxxxxxx>
> ---
> src/qemu/qemu_capabilities.c | 59 ++++++++++++++++++++++++++++++--------------
> src/qemu/qemu_capabilities.h | 3 ++-
> 2 files changed, 42 insertions(+), 20 deletions(-)
>
> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> index 91e8b30..d1c31ad 100644
> --- a/src/qemu/qemu_capabilities.c
> +++ b/src/qemu/qemu_capabilities.c
> @@ -3516,25 +3516,21 @@ virQEMUCapsInitCached(virCapsPtr caps,
> VIR_WARN("Failed to load cached caps from '%s' for '%s': %s",
> capsfile, qemuCaps->binary, virGetLastErrorMessage());
> virResetLastError();
> - ret = 0;
> - virQEMUCapsReset(qemuCaps);
> - goto cleanup;
> + goto discard;
> }
>
> + if (!virQEMUCapsIsValid(qemuCaps, qemuctime))
> + goto discard;
> +
> /* Discard cache if QEMU binary or libvirtd changed */
> - if (qemuctime != qemuCaps->ctime ||
> - selfctime != virGetSelfLastChanged() ||
> + if (selfctime != virGetSelfLastChanged() ||
> selfvers != LIBVIR_VERSION_NUMBER) {
> - VIR_DEBUG("Outdated cached capabilities '%s' for '%s' "
> - "(%lld vs %lld, %lld vs %lld, %lu vs %lu)",
> + VIR_DEBUG("Dropping cached capabilities '%s' for '%s': "
> + "libvirt changed (%lld vs %lld, %lu vs %lu)",
Nice cleanup, the only thing that I would change is to split the DEBUG and move
the "Dropping cached capabilities '%s' for '%s'" and put it ...
> capsfile, qemuCaps->binary,
> - (long long)qemuCaps->ctime, (long long)qemuctime,
> (long long)selfctime, (long long)virGetSelfLastChanged(),
> selfvers, (unsigned long)LIBVIR_VERSION_NUMBER);
> - ignore_value(unlink(capsfile));
> - virQEMUCapsReset(qemuCaps);
> - ret = 0;
> - goto cleanup;
> + goto discard;
> }
>
> VIR_DEBUG("Loaded '%s' for '%s' ctime %lld usedQMP=%d",
> @@ -3548,6 +3544,12 @@ virQEMUCapsInitCached(virCapsPtr caps,
> VIR_FREE(capsfile);
> VIR_FREE(capsdir);
> return ret;
> +
> + discard:
... here where we actually drop the capability.
> + ignore_value(unlink(capsfile));
> + virQEMUCapsReset(qemuCaps);
> + ret = 0;
> + goto cleanup;
> }
>
>
> @@ -4111,17 +4113,36 @@ virQEMUCapsNewForBinary(virCapsPtr caps,
> }
>
>
> -bool virQEMUCapsIsValid(virQEMUCapsPtr qemuCaps)
> +bool
> +virQEMUCapsIsValid(virQEMUCapsPtr qemuCaps,
> + time_t ctime)
> {
> - struct stat sb;
> -
> if (!qemuCaps->binary)
> return true;
>
> - if (stat(qemuCaps->binary, &sb) < 0)
> - return false;
> + if (!ctime) {
> + struct stat sb;
>
> - return sb.st_ctime == qemuCaps->ctime;
> + if (stat(qemuCaps->binary, &sb) < 0) {
> + char ebuf[1024];
> + VIR_DEBUG("Dropping cached capabilities for '%s': "
> + "failed to stat QEMU binary: %s",
> + qemuCaps->binary,
> + virStrerror(errno, ebuf, sizeof(ebuf)));
Same here, remove the "Dropping cached capabilities for '%s': " part and ...
> + return false;
> + }
> + ctime = sb.st_ctime;
> + }
> +
> + if (ctime != qemuCaps->ctime) {
> + VIR_DEBUG("Dropping cached capabilities for '%s': "
> + "binary is newer than cache (%lld vs %lld)",
> + qemuCaps->binary,
> + (long long) ctime, (long long) qemuCaps->ctime);
... same here. This function doesn't drop the capabilities.
ACK, the suggested change is only for debug logs so I'll leave it up to you,
but it would be cleaner :)
Pavel
> + return false;
> + }
> +
> + return true;
> }
>
>
> @@ -4214,7 +4235,7 @@ virQEMUCapsCacheLookup(virCapsPtr caps,
> virMutexLock(&cache->lock);
> ret = virHashLookup(cache->binaries, binary);
> if (ret &&
> - !virQEMUCapsIsValid(ret)) {
> + !virQEMUCapsIsValid(ret, 0)) {
> VIR_DEBUG("Cached capabilities %p no longer valid for %s",
> ret, binary);
> virHashRemoveEntry(cache->binaries, binary);
> diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
> index c77ba13..6c45a67 100644
> --- a/src/qemu/qemu_capabilities.h
> +++ b/src/qemu/qemu_capabilities.h
> @@ -449,7 +449,8 @@ int virQEMUCapsGetMachineTypesCaps(virQEMUCapsPtr qemuCaps,
> size_t *nmachines,
> virCapsGuestMachinePtr **machines);
>
> -bool virQEMUCapsIsValid(virQEMUCapsPtr qemuCaps);
> +bool virQEMUCapsIsValid(virQEMUCapsPtr qemuCaps,
> + time_t ctime);
>
> void virQEMUCapsFilterByMachineType(virQEMUCapsPtr qemuCaps,
> const char *machineType);
> --
> 2.10.2
>
> --
> libvir-list mailing list
> libvir-list@xxxxxxxxxx
> https://www.redhat.com/mailman/listinfo/libvir-list
Attachment:
signature.asc
Description: Digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list