Re: [PATCH] qemu: Only restore security label when saving is successfull.

[Osier Yang <jyang@xxxxxxxxxx>]

ä 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

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list