[libvirt] [PATCH] qemu_capabilities: fix issue with discarding old capabilities
There was a bug that if libvirtd binary has been updated than the capability file wasn't reloaded therefore new capabilities introduced in libvirt cannot be used because the cached version was loaded. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1135431 Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_capabilities.c | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 81ada48..dae89aa 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -286,6 +286,7 @@ struct _virQEMUCaps { char *binary; time_t ctime; + time_t selfctime; virBitmapPtr flags; @@ -2689,7 +2690,7 @@ virQEMUCapsSaveCache(virQEMUCapsPtr qemuCaps, const char *filename) virBufferAsprintf(&buf, "<qemuctime>%llu</qemuctime>\n", (long long)qemuCaps->ctime); virBufferAsprintf(&buf, "<selfctime>%llu</selfctime>\n", - (long long)virGetSelfLastChanged()); + (long long)qemuCaps->selfctime); if (qemuCaps->usedQMP) virBufferAddLit(&buf, "<usedQMP/>\n"); @@ -2743,7 +2744,7 @@ virQEMUCapsSaveCache(virQEMUCapsPtr qemuCaps, const char *filename) VIR_DEBUG("Saved caps '%s' for '%s' with (%lld, %lld)", filename, qemuCaps->binary, (long long)qemuCaps->ctime, - (long long)virGetSelfLastChanged()); + (long long)qemuCaps->selfctime); ret = 0; cleanup: @@ -2871,12 +2872,12 @@ virQEMUCapsInitCached(virQEMUCapsPtr qemuCaps, const char *cacheDir) /* Discard if cache is older that QEMU binary */ if (qemuctime != qemuCaps->ctime || - selfctime < virGetSelfLastChanged()) { + selfctime < qemuCaps->selfctime) { VIR_DEBUG("Outdated cached capabilities '%s' for '%s' " "(%lld vs %lld, %lld vs %lld)", capsfile, qemuCaps->binary, (long long)qemuctime, (long long)qemuCaps->ctime, - (long long)selfctime, (long long)virGetSelfLastChanged()); + (long long)selfctime, (long long)qemuCaps->selfctime); ignore_value(unlink(capsfile)); virQEMUCapsReset(qemuCaps); ret = 0; @@ -3371,6 +3372,7 @@ virQEMUCapsPtr virQEMUCapsNewForBinary(const char *binary, goto error; } qemuCaps->ctime = sb.st_ctime; + qemuCaps->selfctime = virGetSelfLastChanged(); /* Make sure the binary we are about to try exec'ing exists. * Technically we could catch the exec() failure, but that's @@ -3420,7 +3422,8 @@ bool virQEMUCapsIsValid(virQEMUCapsPtr qemuCaps) if (stat(qemuCaps->binary, &sb) < 0) return false; - return sb.st_ctime == qemuCaps->ctime; + return sb.st_ctime == qemuCaps->ctime && + virGetSelfLastChanged() >= qemuCaps->selfctime; } -- 1.8.5.5
On Fri, Sep 12, 2014 at 06:10:44PM +0200, Pavel Hrdina wrote:
There was a bug that if libvirtd binary has been updated than the capability file wasn't reloaded therefore new capabilities introduced in libvirt cannot be used because the cached version was loaded.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1135431
That bug is all about FIPS support.
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 81ada48..dae89aa 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -286,6 +286,7 @@ struct _virQEMUCaps {
char *binary; time_t ctime; + time_t selfctime;
virBitmapPtr flags;
@@ -2689,7 +2690,7 @@ virQEMUCapsSaveCache(virQEMUCapsPtr qemuCaps, const char *filename) virBufferAsprintf(&buf, "<qemuctime>%llu</qemuctime>\n", (long long)qemuCaps->ctime); virBufferAsprintf(&buf, "<selfctime>%llu</selfctime>\n", - (long long)virGetSelfLastChanged()); + (long long)qemuCaps->selfctime);
if (qemuCaps->usedQMP) virBufferAddLit(&buf, "<usedQMP/>\n"); @@ -2743,7 +2744,7 @@ virQEMUCapsSaveCache(virQEMUCapsPtr qemuCaps, const char *filename) VIR_DEBUG("Saved caps '%s' for '%s' with (%lld, %lld)", filename, qemuCaps->binary, (long long)qemuCaps->ctime, - (long long)virGetSelfLastChanged()); + (long long)qemuCaps->selfctime);
ret = 0; cleanup: @@ -2871,12 +2872,12 @@ virQEMUCapsInitCached(virQEMUCapsPtr qemuCaps, const char *cacheDir)
/* Discard if cache is older that QEMU binary */ if (qemuctime != qemuCaps->ctime || - selfctime < virGetSelfLastChanged()) { + selfctime < qemuCaps->selfctime) { VIR_DEBUG("Outdated cached capabilities '%s' for '%s' " "(%lld vs %lld, %lld vs %lld)", capsfile, qemuCaps->binary, (long long)qemuctime, (long long)qemuCaps->ctime, - (long long)selfctime, (long long)virGetSelfLastChanged()); + (long long)selfctime, (long long)qemuCaps->selfctime); ignore_value(unlink(capsfile)); virQEMUCapsReset(qemuCaps); ret = 0; @@ -3371,6 +3372,7 @@ virQEMUCapsPtr virQEMUCapsNewForBinary(const char *binary, goto error; } qemuCaps->ctime = sb.st_ctime; + qemuCaps->selfctime = virGetSelfLastChanged();
/* Make sure the binary we are about to try exec'ing exists. * Technically we could catch the exec() failure, but that's @@ -3420,7 +3422,8 @@ bool virQEMUCapsIsValid(virQEMUCapsPtr qemuCaps) if (stat(qemuCaps->binary, &sb) < 0) return false;
- return sb.st_ctime == qemuCaps->ctime; + return sb.st_ctime == qemuCaps->ctime && + virGetSelfLastChanged() >= qemuCaps->selfctime; }
Huh, this doesn't make any sense. The virQEMUCapsIsValid() method is used to invalidate the cache if QEMU binary changes while libvirtd is running. It is not relevant whether libvirtd itself has changed here, because changes to the libvirtd binary on disk have no effect until libvirtd is restarted. When libvirtd is (re)started, the virQEMUCapsInitCached will already check the cache selfctime vs its own virGetSelfLastChanged() and throw away the cache file if stale. So I don't see any bug here that needs fixing. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 09/12/2014 06:25 PM, Daniel P. Berrange wrote:
On Fri, Sep 12, 2014 at 06:10:44PM +0200, Pavel Hrdina wrote:
There was a bug that if libvirtd binary has been updated than the capability file wasn't reloaded therefore new capabilities introduced in libvirt cannot be used because the cached version was loaded.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1135431
That bug is all about FIPS support.
Yes it's about FIPS support but it's already in libvirt. I've tested it and actually by removing cached file to force detect new capabilities and after that it worked. Now I realized that even checking the selfctime during start of libvirtd isn't sufficient because you can enable the FIPS support for kenrel without updating the libvirtd binary.
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 81ada48..dae89aa 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -286,6 +286,7 @@ struct _virQEMUCaps {
char *binary; time_t ctime; + time_t selfctime;
virBitmapPtr flags;
@@ -2689,7 +2690,7 @@ virQEMUCapsSaveCache(virQEMUCapsPtr qemuCaps, const char *filename) virBufferAsprintf(&buf, "<qemuctime>%llu</qemuctime>\n", (long long)qemuCaps->ctime); virBufferAsprintf(&buf, "<selfctime>%llu</selfctime>\n", - (long long)virGetSelfLastChanged()); + (long long)qemuCaps->selfctime);
if (qemuCaps->usedQMP) virBufferAddLit(&buf, "<usedQMP/>\n"); @@ -2743,7 +2744,7 @@ virQEMUCapsSaveCache(virQEMUCapsPtr qemuCaps, const char *filename) VIR_DEBUG("Saved caps '%s' for '%s' with (%lld, %lld)", filename, qemuCaps->binary, (long long)qemuCaps->ctime, - (long long)virGetSelfLastChanged()); + (long long)qemuCaps->selfctime);
ret = 0; cleanup: @@ -2871,12 +2872,12 @@ virQEMUCapsInitCached(virQEMUCapsPtr qemuCaps, const char *cacheDir)
/* Discard if cache is older that QEMU binary */ if (qemuctime != qemuCaps->ctime || - selfctime < virGetSelfLastChanged()) { + selfctime < qemuCaps->selfctime) { VIR_DEBUG("Outdated cached capabilities '%s' for '%s' " "(%lld vs %lld, %lld vs %lld)", capsfile, qemuCaps->binary, (long long)qemuctime, (long long)qemuCaps->ctime, - (long long)selfctime, (long long)virGetSelfLastChanged()); + (long long)selfctime, (long long)qemuCaps->selfctime); ignore_value(unlink(capsfile)); virQEMUCapsReset(qemuCaps); ret = 0; @@ -3371,6 +3372,7 @@ virQEMUCapsPtr virQEMUCapsNewForBinary(const char *binary, goto error; } qemuCaps->ctime = sb.st_ctime; + qemuCaps->selfctime = virGetSelfLastChanged();
/* Make sure the binary we are about to try exec'ing exists. * Technically we could catch the exec() failure, but that's @@ -3420,7 +3422,8 @@ bool virQEMUCapsIsValid(virQEMUCapsPtr qemuCaps) if (stat(qemuCaps->binary, &sb) < 0) return false;
- return sb.st_ctime == qemuCaps->ctime; + return sb.st_ctime == qemuCaps->ctime && + virGetSelfLastChanged() >= qemuCaps->selfctime; }
Huh, this doesn't make any sense.
The virQEMUCapsIsValid() method is used to invalidate the cache if QEMU binary changes while libvirtd is running. It is not relevant whether libvirtd itself has changed here, because changes to the libvirtd binary on disk have no effect until libvirtd is restarted.
That's a good point and I've missed that.
When libvirtd is (re)started, the virQEMUCapsInitCached will already check the cache selfctime vs its own virGetSelfLastChanged() and throw away the cache file if stale.
As I've wrote above, there is still issue with the FIPS that it can be enabled without updating libvirtd. And yes you are right that QEMU should autodetect it, but it seems they are not planning to do anything about that and probably we have to deal with it and somehow make it work. I'll try to come up with different patch. Thanks, Pavel
So I don't see any bug here that needs fixing.
Regards, Daniel
On Fri, Sep 12, 2014 at 06:42:08PM +0200, Pavel Hrdina wrote:
On 09/12/2014 06:25 PM, Daniel P. Berrange wrote:
On Fri, Sep 12, 2014 at 06:10:44PM +0200, Pavel Hrdina wrote:
There was a bug that if libvirtd binary has been updated than the capability file wasn't reloaded therefore new capabilities introduced in libvirt cannot be used because the cached version was loaded.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1135431
That bug is all about FIPS support.
Yes it's about FIPS support but it's already in libvirt. I've tested it and actually by removing cached file to force detect new capabilities and after that it worked.
Now I realized that even checking the selfctime during start of libvirtd isn't sufficient because you can enable the FIPS support for kenrel without updating the libvirtd binary.
Ah, so the actual bug is that the capabilities we detect have a dependancy on (libvirtd binary, qemu binary, sysfs/procfs settings). It is pretty difficult to deal with sysfs/procfs chances & caching here, since there's no way I know to detect when sysfs/procfs settings change. I wouldn't want to check the sysfs/procfs settings every time. Perhaps it would suffice to just do a check on sysfs/procfs when libvirtd starts up, so we can say that if you change FIPS sysfs settings you must restart libvirtd ? Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 09/15/2014 11:24 AM, Daniel P. Berrange wrote:
On Fri, Sep 12, 2014 at 06:42:08PM +0200, Pavel Hrdina wrote:
On 09/12/2014 06:25 PM, Daniel P. Berrange wrote:
On Fri, Sep 12, 2014 at 06:10:44PM +0200, Pavel Hrdina wrote:
There was a bug that if libvirtd binary has been updated than the capability file wasn't reloaded therefore new capabilities introduced in libvirt cannot be used because the cached version was loaded.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1135431
That bug is all about FIPS support.
Yes it's about FIPS support but it's already in libvirt. I've tested it and actually by removing cached file to force detect new capabilities and after that it worked.
Now I realized that even checking the selfctime during start of libvirtd isn't sufficient because you can enable the FIPS support for kenrel without updating the libvirtd binary.
Ah, so the actual bug is that the capabilities we detect have a dependancy on (libvirtd binary, qemu binary, sysfs/procfs settings). It is pretty difficult to deal with sysfs/procfs chances & caching here, since there's no way I know to detect when sysfs/procfs settings change.
Yes, that's the real bug and I also didn't realize that at first. There is however one more think I'm not sure about. I didn't find any place where we are discarding old capabilities if the libvirtd binary has been changed. The only check for that update is in function "virQEMUCapsInitCached" and its called only from "virQEMUCapsNewForBinary" and this function is called only if there is no cached caps or the qemu binary has changed. See the "virQEMUCapsCacheLookup". So it seems that there is also a bug that we don't check on libvirtd start if there was an update of that binary.
I wouldn't want to check the sysfs/procfs settings every time. Perhaps it would suffice to just do a check on sysfs/procfs when libvirtd starts up, so we can say that if you change FIPS sysfs settings you must restart libvirtd ?
I think that would be good enough. Pavel
Regards, Daniel
On Mon, Sep 15, 2014 at 11:43:10AM +0200, Pavel Hrdina wrote:
On 09/15/2014 11:24 AM, Daniel P. Berrange wrote:
On Fri, Sep 12, 2014 at 06:42:08PM +0200, Pavel Hrdina wrote:
On 09/12/2014 06:25 PM, Daniel P. Berrange wrote:
On Fri, Sep 12, 2014 at 06:10:44PM +0200, Pavel Hrdina wrote:
There was a bug that if libvirtd binary has been updated than the capability file wasn't reloaded therefore new capabilities introduced in libvirt cannot be used because the cached version was loaded.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1135431
That bug is all about FIPS support.
Yes it's about FIPS support but it's already in libvirt. I've tested it and actually by removing cached file to force detect new capabilities and after that it worked.
Now I realized that even checking the selfctime during start of libvirtd isn't sufficient because you can enable the FIPS support for kenrel without updating the libvirtd binary.
Ah, so the actual bug is that the capabilities we detect have a dependancy on (libvirtd binary, qemu binary, sysfs/procfs settings). It is pretty difficult to deal with sysfs/procfs chances & caching here, since there's no way I know to detect when sysfs/procfs settings change.
Yes, that's the real bug and I also didn't realize that at first.
There is however one more think I'm not sure about. I didn't find any place where we are discarding old capabilities if the libvirtd binary has been changed. The only check for that update is in function "virQEMUCapsInitCached" and its called only from "virQEMUCapsNewForBinary" and this function is called only if there is no cached caps or the qemu binary has changed. See the "virQEMUCapsCacheLookup".
So it seems that there is also a bug that we don't check on libvirtd start if there was an update of that binary.
When libvirtd starts up the cache will be empty. So virQEMUCapsCacheLookup will always call virQEMUCapsNewForBinary which will call virQEMUCapsInitCached. So it will always check timestamps on startup.
I wouldn't want to check the sysfs/procfs settings every time. Perhaps it would suffice to just do a check on sysfs/procfs when libvirtd starts up, so we can say that if you change FIPS sysfs settings you must restart libvirtd ?
I think that would be good enough.
The difficultly though is figuring out whehther the files have changed. For this, we'd need to record what the original sysfs/procfs values were in the caps cache, so we then have something to compare. A completely different way of looking at this problem would be to say that the virQEMUCapsPtr should *only* reflect settings that are related to the QEMU binary capabilities. ie sysfs/procs should not be allowed to influence the capabilities flags. This is kind of what was originally assumed for this data. I wonder how many of our capability flags are set based on data that is not from the QEMU binary ? If we can elminate that, then the caching problem will go away. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 09/15/2014 11:49 AM, Daniel P. Berrange wrote:
On Mon, Sep 15, 2014 at 11:43:10AM +0200, Pavel Hrdina wrote:
On 09/15/2014 11:24 AM, Daniel P. Berrange wrote:
On Fri, Sep 12, 2014 at 06:42:08PM +0200, Pavel Hrdina wrote:
On 09/12/2014 06:25 PM, Daniel P. Berrange wrote:
On Fri, Sep 12, 2014 at 06:10:44PM +0200, Pavel Hrdina wrote:
There was a bug that if libvirtd binary has been updated than the capability file wasn't reloaded therefore new capabilities introduced in libvirt cannot be used because the cached version was loaded.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1135431
That bug is all about FIPS support.
Yes it's about FIPS support but it's already in libvirt. I've tested it and actually by removing cached file to force detect new capabilities and after that it worked.
Now I realized that even checking the selfctime during start of libvirtd isn't sufficient because you can enable the FIPS support for kenrel without updating the libvirtd binary.
Ah, so the actual bug is that the capabilities we detect have a dependancy on (libvirtd binary, qemu binary, sysfs/procfs settings). It is pretty difficult to deal with sysfs/procfs chances & caching here, since there's no way I know to detect when sysfs/procfs settings change.
Yes, that's the real bug and I also didn't realize that at first.
There is however one more think I'm not sure about. I didn't find any place where we are discarding old capabilities if the libvirtd binary has been changed. The only check for that update is in function "virQEMUCapsInitCached" and its called only from "virQEMUCapsNewForBinary" and this function is called only if there is no cached caps or the qemu binary has changed. See the "virQEMUCapsCacheLookup".
So it seems that there is also a bug that we don't check on libvirtd start if there was an update of that binary.
When libvirtd starts up the cache will be empty. So virQEMUCapsCacheLookup will always call virQEMUCapsNewForBinary which will call virQEMUCapsInitCached. So it will always check timestamps on startup.
Well, that's not true. The cache file survive libvirtd stop/start and if there is existing cache file, it will be dropped only if qemu binary has been changed.
I wouldn't want to check the sysfs/procfs settings every time. Perhaps it would suffice to just do a check on sysfs/procfs when libvirtd starts up, so we can say that if you change FIPS sysfs settings you must restart libvirtd ?
I think that would be good enough.
The difficultly though is figuring out whehther the files have changed. For this, we'd need to record what the original sysfs/procfs values were in the caps cache, so we then have something to compare.
A completely different way of looking at this problem would be to say that the virQEMUCapsPtr should *only* reflect settings that are related to the QEMU binary capabilities. ie sysfs/procs should not be allowed to influence the capabilities flags. This is kind of what was originally assumed for this data.
I wonder how many of our capability flags are set based on data that is not from the QEMU binary ? If we can elminate that, then the caching problem will go away.
It seems that the FIPS is the only case that shouldn't be in qemuCaps. I'll create a new patch where I'll move that code directly to qemu_command. Pavel
Regards, Daniel
participants (2)
-
Daniel P. Berrange -
Pavel Hrdina