3:54 a.m.
"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
10:54 a.m.
New subject: [libvirt] [PATCH] qemu: Only restore security label when saving is successfull.
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(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
11:16 p.m.
New subject: [libvirt] [PATCH] qemu: Only restore security label when saving is successfull.
于 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
2:07 p.m.
New subject: [libvirt] [PATCH] qemu: Only restore security label when saving is successfull.
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(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
10:35 p.m.
New subject: [libvirt] [PATCH] qemu: Only restore security label when saving is successfull.
于 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
4987
days inactive
4991
days old
4 comments
2 participants
participants (2)
-
Eric Blake -
Osier Yang