On Mon, Sep 16, 2019 at 16:13:39 +0200, Peter Krempa wrote:
> On Mon, Sep 16, 2019 at 12:34:32 +0200, Michal Privoznik wrote:
> > Turns out, block mirror is not the only job a disk can have. It
> > can also do commits of one layer into the other. Or possibly some
> > other tricks too. Problem is that while we set seclabels on given
> > layers of backing chain when the job is starting (via
> > qemuDomainStorageSourceAccessAllow()) we don't restore them when
> > job finishes. This leaves XATTRs set and corresponding images
> > unusable.
> >
> > Signed-off-by: Michal Privoznik <mprivozn@xxxxxxxxxx>
> > ---
> >
> > Not sure if we want to remove XATTRs for the top layer too or just the
> > rest of the backing chain (n=disk->src vs. n=disk->src->backingStore).
> > Peter?
>
> That depends on the job. In case of the legacy handler both job types
> which modify which top level image (thus the image which is stored as
> disk->src will no longer be used after the handler returns) is being
> used are handled via the
> "if (disk->mirrorState == VIR_DOMAIN_DISK_MIRROR_STATE_PIVOT) {"
> code path.
>
> That case already handles the top level image where the metadata is
> transferred, but both block copy and active block commit may remove a
> chain of images from use.
>
> All other cases are handled via the '} else {' branch:
>
> In case of block pull, libvirt does not need to modify any permissions
> as the data is pulled into the top level image directly.
>
> In case of (non active layer) block commit (assume the following backing
> chain for example's sake: #activeimage -> #dummy1 -> #top ->
> #intermediate1..n -> #base -> #endofchain). Libvirt modifies #dummy1's
> and #base's permissions to read write. #dummy1 gets the backing store
> specifier updated and #base gets all the data. Then #top and all
> #intermediateH images are dropped from the chain. #activeimage stays
> read-write.
>
> This means that we probably shouldn't modify/drop labels for
> #activeimage (disk->src) as it will be used same way it was.
>
> Note that there's still the question of failed cancelled jobs. We still
> do the relabelling when starting the job and the images are still used,
> the question is whether it's cleaned up during VM shutdown.
>
> >
> > src/qemu/qemu_blockjob.c | 14 ++++++++++++--
> > 1 file changed, 12 insertions(+), 2 deletions(-)
> >
> > diff --git a/src/qemu/qemu_blockjob.c b/src/qemu/qemu_blockjob.c
> > index a991309ee7..6408f95e4e 100644
> > --- a/src/qemu/qemu_blockjob.c
> > +++ b/src/qemu/qemu_blockjob.c
> > @@ -658,9 +658,9 @@ qemuBlockJobEventProcessLegacyCompleted(virQEMUDriverPtr driver,
> > virObjectUnref(disk->src);
> > disk->src = disk->mirror;
> > } else {
> > + virStorageSourcePtr n;
> > +
> > if (disk->mirror) {
> > - virStorageSourcePtr n;
> > -
> > virDomainLockImageDetach(driver->lockManager, vm, disk->mirror);
> >
> > /* Ideally, we would restore seclabels on the backing chain here
> > @@ -678,6 +678,16 @@ qemuBlockJobEventProcessLegacyCompleted(virQEMUDriverPtr driver,
> >
> > virObjectUnref(disk->mirror);
> > }
> > +
> > + for (n = disk->src; virStorageSourceIsBacking(n); n = n->backingStore) {
>
> disk->src is probably okay to stay as-is.
>
> > + if (qemuSecurityMoveImageMetadata(driver, vm, n, NULL) < 0) {
> > + VIR_WARN("Unable to remove disk metadata on "
> > + "vm %s from %s (disk target %s)",
> > + vm->def->name,
> > + NULLSTR(n->path),
> > + disk->dst);
> > + }
> > + }
> > }
> >
> > /* Recompute the cached backing chain to match our
> > --
ACK as I've forgot to add it here.
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
