[libvirt] [PATCH] qemu: Only restore security label when saving is successfull.
"qemudDomainSaveFlag" trys to restore security label even if the saving fails, a useless warning will be thowed then, e.g. if "doStopVcpus" fails. * src/qemu/qemu_driver.c --- src/qemu/qemu_driver.c | 5 ++++- 1 files changed, 4 insertions(+), 1 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index af897ad..1baee58 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -1823,6 +1823,7 @@ static int qemudDomainSaveFlag(struct qemud_driver *driver, virDomainPtr dom, int is_reg = 0; unsigned long long offset; virCgroupPtr cgroup = NULL; + bool saved = false; memset(&header, 0, sizeof(header)); memcpy(header.magic, QEMUD_SAVE_MAGIC, sizeof(header.magic)); @@ -2040,6 +2041,8 @@ static int qemudDomainSaveFlag(struct qemud_driver *driver, virDomainPtr dom, if (rc < 0) goto endjob; + saved = true; + if ((!bypassSecurityDriver) && virSecurityManagerRestoreSavedStateLabel(driver->securityManager, vm, path) < 0) @@ -2087,7 +2090,7 @@ endjob: path, vm->def->name, rc); } - if ((!bypassSecurityDriver) && + if ((!bypassSecurityDriver) && saved && virSecurityManagerRestoreSavedStateLabel(driver->securityManager, vm, path) < 0) VIR_WARN("failed to restore save state label on %s", path); -- 1.7.4
On 03/25/2011 02:54 AM, Osier Yang wrote:
"qemudDomainSaveFlag" trys to restore security label even if the saving fails, a useless warning will be thowed then, e.g. if "doStopVcpus" fails.
* src/qemu/qemu_driver.c --- src/qemu/qemu_driver.c | 5 ++++- 1 files changed, 4 insertions(+), 1 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index af897ad..1baee58 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -1823,6 +1823,7 @@ static int qemudDomainSaveFlag(struct qemud_driver *driver, virDomainPtr dom, int is_reg = 0; unsigned long long offset; virCgroupPtr cgroup = NULL; + bool saved = false;
You don't need this if we can key off of some other condition.
memset(&header, 0, sizeof(header)); memcpy(header.magic, QEMUD_SAVE_MAGIC, sizeof(header.magic)); @@ -2040,6 +2041,8 @@ static int qemudDomainSaveFlag(struct qemud_driver *driver, virDomainPtr dom, if (rc < 0) goto endjob;
+ saved = true; +
Therefore we don't need this.
if ((!bypassSecurityDriver) && virSecurityManagerRestoreSavedStateLabel(driver->securityManager, vm, path) < 0)
Rather, after the point where you have first attempted RestoreSavedStateLabel, merely change bypassSecurityDriver to true, to avoid...
@@ -2087,7 +2090,7 @@ endjob: path, vm->def->name, rc); }
- if ((!bypassSecurityDriver) && + if ((!bypassSecurityDriver) && saved && virSecurityManagerRestoreSavedStateLabel(driver->securityManager,
a second attempt at restoring the label after the first one. Besides, your logic looks wrong - you are now attempting the second restore only if 'saved' is true, where in reality, you want to attempt the second restore only if the first restore wasn't attempted, or '!saved'. In other words, I think this one-liner is a better patch: diff --git i/src/qemu/qemu_driver.c w/src/qemu/qemu_driver.c index af897ad..514ff78 100644 --- i/src/qemu/qemu_driver.c +++ w/src/qemu/qemu_driver.c @@ -2044,6 +2044,7 @@ static int qemudDomainSaveFlag(struct qemud_driver *driver, virDomainPtr dom, virSecurityManagerRestoreSavedStateLabel(driver->securityManager, vm, path) < 0) VIR_WARN("failed to restore save state label on %s", path); + bypassSecurityDriver = true; if (cgroup != NULL) { rc = virCgroupDenyDevicePath(cgroup, path, -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
于 2011年03月25日 23:54, Eric Blake 写道:
On 03/25/2011 02:54 AM, Osier Yang wrote:
"qemudDomainSaveFlag" trys to restore security label even if the saving fails, a useless warning will be thowed then, e.g. if "doStopVcpus" fails.
* src/qemu/qemu_driver.c --- src/qemu/qemu_driver.c | 5 ++++- 1 files changed, 4 insertions(+), 1 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index af897ad..1baee58 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -1823,6 +1823,7 @@ static int qemudDomainSaveFlag(struct qemud_driver *driver, virDomainPtr dom, int is_reg = 0; unsigned long long offset; virCgroupPtr cgroup = NULL; + bool saved = false;
You don't need this if we can key off of some other condition.
memset(&header, 0, sizeof(header)); memcpy(header.magic, QEMUD_SAVE_MAGIC, sizeof(header.magic)); @@ -2040,6 +2041,8 @@ static int qemudDomainSaveFlag(struct qemud_driver *driver, virDomainPtr dom, if (rc< 0) goto endjob;
+ saved = true; +
Therefore we don't need this.
if ((!bypassSecurityDriver)&& virSecurityManagerRestoreSavedStateLabel(driver->securityManager, vm, path)< 0)
Rather, after the point where you have first attempted RestoreSavedStateLabel, merely change bypassSecurityDriver to true, to avoid...
@@ -2087,7 +2090,7 @@ endjob: path, vm->def->name, rc); }
- if ((!bypassSecurityDriver)&& + if ((!bypassSecurityDriver)&& saved&& virSecurityManagerRestoreSavedStateLabel(driver->securityManager,
a second attempt at restoring the label after the first one.
Besides, your logic looks wrong - you are now attempting the second restore only if 'saved' is true, where in reality, you want to attempt the second restore only if the first restore wasn't attempted, or '!saved'.
In other words, I think this one-liner is a better patch:
diff --git i/src/qemu/qemu_driver.c w/src/qemu/qemu_driver.c index af897ad..514ff78 100644 --- i/src/qemu/qemu_driver.c +++ w/src/qemu/qemu_driver.c @@ -2044,6 +2044,7 @@ static int qemudDomainSaveFlag(struct qemud_driver *driver, virDomainPtr dom, virSecurityManagerRestoreSavedStateLabel(driver->securityManager, vm, path)< 0) VIR_WARN("failed to restore save state label on %s", path); + bypassSecurityDriver = true;
No, that's not the original patch meant, and this attached patch doesn't fix the problem. /* Pause */ if (vm->state == VIR_DOMAIN_RUNNING) { header.was_running = 1; if (qemuProcessStopCPUs(driver, vm) < 0) goto endjob; The original patch meant: In case of "qemuProcessStopCPUs" failure, (in RHEL, it's "doStopVcpus"), which means VM is still not NULL, and "bypassSecurityDriver" is never changed before, (it's initialized as 0 at the beginning), so, when it jumps to "endjob", it must try to restore the label for the saving path, however, it even didn't try to save (even no label setting before), as a result, it will always warn "No such file or directory, bla bla". So changing "bypassSecurityDriver" after the first restoring attempt doesn't work. Regards Osier
On 03/27/2011 10:16 PM, Osier Yang wrote:
于 2011年03月25日 23:54, Eric Blake 写道:
On 03/25/2011 02:54 AM, Osier Yang wrote:
"qemudDomainSaveFlag" trys to restore security label even if the saving fails, a useless warning will be thowed then, e.g. if "doStopVcpus" fails.
* src/qemu/qemu_driver.c ---
virCgroupPtr cgroup = NULL; + bool saved = false;
You don't need this if we can key off of some other condition.
No, that's not the original patch meant, and this attached patch doesn't fix the problem.
/* Pause */ if (vm->state == VIR_DOMAIN_RUNNING) { header.was_running = 1; if (qemuProcessStopCPUs(driver, vm) < 0) goto endjob;
The original patch meant:
In case of "qemuProcessStopCPUs" failure, (in RHEL, it's "doStopVcpus"), which means VM is still not NULL, and "bypassSecurityDriver" is never changed before, (it's initialized as 0 at the beginning), so, when it jumps to "endjob", it must try to restore the label for the saving path, however, it even didn't try to save (even no label setting before), as a result, it will always warn "No such file or directory, bla bla".
So changing "bypassSecurityDriver" after the first restoring attempt doesn't work.
Aha - I see what you mean. We had _two_ logic bugs, and I only fixed the second one (we restored the label twice on a late error), whereas you were trying to fix the first (we restore the label even if we never set it in the first place on an early error). Meanwhile, my fd: migration already touched this file, so your patch no longer applies. I think I fixed that incidentally by moving the labeling into qemu_migration.c (that is, commit 6034ddd55 should fix the issue you were first testing). Now, the bypassSecurityDriver in qemu_driver.c doesn't affect any cleanup paths early or late, and in the new qemuMigrationToFile, bypassSecurityDriver is coupled with the new restoreLabel boolean which is set only when the labeling took place, so the cleanup is properly gated. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
于 2011年03月29日 03:07, Eric Blake 写道:
On 03/27/2011 10:16 PM, Osier Yang wrote:
于 2011年03月25日 23:54, Eric Blake 写道:
On 03/25/2011 02:54 AM, Osier Yang wrote:
"qemudDomainSaveFlag" trys to restore security label even if the saving fails, a useless warning will be thowed then, e.g. if "doStopVcpus" fails.
* src/qemu/qemu_driver.c ---
virCgroupPtr cgroup = NULL; + bool saved = false;
You don't need this if we can key off of some other condition.
No, that's not the original patch meant, and this attached patch doesn't fix the problem.
/* Pause */ if (vm->state == VIR_DOMAIN_RUNNING) { header.was_running = 1; if (qemuProcessStopCPUs(driver, vm)< 0) goto endjob;
The original patch meant:
In case of "qemuProcessStopCPUs" failure, (in RHEL, it's "doStopVcpus"), which means VM is still not NULL, and "bypassSecurityDriver" is never changed before, (it's initialized as 0 at the beginning), so, when it jumps to "endjob", it must try to restore the label for the saving path, however, it even didn't try to save (even no label setting before), as a result, it will always warn "No such file or directory, bla bla".
So changing "bypassSecurityDriver" after the first restoring attempt doesn't work.
Aha - I see what you mean. We had _two_ logic bugs, and I only fixed the second one (we restored the label twice on a late error), whereas you were trying to fix the first (we restore the label even if we never set it in the first place on an early error).
Meanwhile, my fd: migration already touched this file, so your patch no longer applies. I think I fixed that incidentally by moving the labeling into qemu_migration.c (that is, commit 6034ddd55 should fix the issue you were first testing). Now, the bypassSecurityDriver in qemu_driver.c doesn't affect any cleanup paths early or late, and in the new qemuMigrationToFile, bypassSecurityDriver is coupled with the new restoreLabel boolean which is set only when the labeling took place, so the cleanup is properly gated.
I looked through commit 6034ddd55, it should fix the problem, thanks for picking it up incidentally. Osier
participants (2)
-
Eric Blake -
Osier Yang