On Mon, Jan 28, 2019 at 13:00:03 -0500, John Ferlan wrote:
>
>
> On 1/23/19 11:11 AM, Peter Krempa wrote:
> > Be more sensible when setting labels of the target of a
> > virDomainBlockCopy operation. Previously we'd relabel everything in case
> > it's a copy job even if there's no unlabelled backing chain. Since we
> > are also not sure whether the backing chain is shared we don't relabel
> > the chain on completion of the blockjob. This certainly won't play nice
> > with the image permission relabelling feature.
> >
> > While this does not fix the case where the image is reused and has
> > backing chain it certainly sanitizes all the other cases. Later on it
> > will also allow to do the correct thing in cases where only one layer
> > was introduced.
I also forgot to note:
The change is necessary as in case when -blockdev will be used
we will need to hotplug the backing chain and thus labeling
needs to be setup in advance and not only at the time of pivot.
To avoid multiple code paths move the labeling now.
> >
> > Signed-off-by: Peter Krempa <pkrempa@xxxxxxxxxx>
> > ---
> > src/qemu/qemu_driver.c | 43 ++++++++++++++++++++---------------------
> > src/qemu/qemu_process.c | 22 +++++++++++++++++++++
> > 2 files changed, 43 insertions(+), 22 deletions(-)
> >
>
> [sorry - got sidetracked by a class...]
>
> I get to this point and started wondering - hmm... are there any
> assumptions or issues w/ [managed] save files? But I guess that what
> the changes in qemuProcessRefreshLegacyBlockjob will handle, right?
Saving vms, migration and all the other stuff are forbidden when
blockjobs are running as we can't restore them afterwards.
I'm not sure whether it makes much sense to bother with restoring the
job state in these cases as the use cases are really slim.
>
> > diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> > index 79a767288e..2c2c0ce92e 100644
> > --- a/src/qemu/qemu_driver.c
> > +++ b/src/qemu/qemu_driver.c
> > @@ -17170,26 +17170,6 @@ qemuDomainBlockPivot(virQEMUDriverPtr driver,
> > goto cleanup;
> > }
> >
> > - /* For active commit, the mirror is part of the already labeled
> > - * chain. For blockcopy, we previously labeled only the top-level
> > - * image; but if the user is reusing an external image that
> > - * includes a backing file, the pivot may result in qemu needing
> > - * to open the entire backing chain, so we need to label the
> > - * entire chain. This action is safe even if the backing chain
> > - * has already been labeled; but only necessary when we know for
> > - * sure that there is a backing chain. */
> > - if (disk->mirrorJob == VIR_DOMAIN_BLOCK_JOB_TYPE_COPY) {
> > - if (qemuDomainDetermineDiskChain(driver, vm, disk, disk->mirror, true) < 0)
> > - goto cleanup;
> > -
> > - if (disk->mirror->format &&
> > - disk->mirror->format != VIR_STORAGE_FILE_RAW &&
> > - (qemuDomainNamespaceSetupDisk(vm, disk->mirror) < 0 ||
> > - qemuSetupImageChainCgroup(vm, disk->mirror) < 0 ||
> > - qemuSecuritySetImageLabel(driver, vm, disk->mirror, true) < 0))
> > - goto cleanup;
> > - }
> > -
> > /* Attempt the pivot. Record the attempt now, to prevent duplicate
> > * attempts; but the actual disk change will be made when emitting
> > * the event.
> > @@ -17836,9 +17816,28 @@ qemuDomainBlockCopyCommon(virDomainObjPtr vm,
> > keepParentLabel) < 0)
> > goto endjob;
> >
> > - if (qemuDomainDiskChainElementPrepare(driver, vm, mirror, false, true) < 0) {
> > - qemuDomainDiskChainElementRevoke(driver, vm, mirror);
> > + /* If reusing an external image that includes a backing file, the pivot may
> > + * result in qemu needing to open the entire backing chain, so we need to
> > + * label the full backing chain of the mirror instead of just the top image */
> > + if (flags & VIR_DOMAIN_BLOCK_COPY_REUSE_EXT &&
> > + mirror->format >= VIR_STORAGE_FILE_BACKING &&
> > + qemuDomainDetermineDiskChain(driver, vm, disk, mirror, true) < 0)
> > goto endjob;
> > +
> > + if (flags & VIR_DOMAIN_BLOCK_COPY_REUSE_EXT &&
> > + virStorageSourceHasBacking(mirror)) {
> > + /* note that we don't really know whether a part of the backing chain
> > + * is shared so rolling this back is not as easy. Thus we do it only
> > + * if there's a backing chain */
> > + if (qemuDomainNamespaceSetupDisk(vm, mirror) < 0 ||
> > + qemuSetupImageChainCgroup(vm, disk->mirror) < 0 ||
> > + qemuSecuritySetImageLabel(driver, vm, disk->mirror, true) < 0)
>
> The words in the comment don't really tell me whether @mirror or
> @disk->mirror is expected to be passed along at least for the second two
> uses. Since you know the code better, I'll defer to your judgment. It
> seems logical, since IIUC the code is copying to @mirror and
> having/setting cgroup/imagelabel on disk->mirror would seem to be the
> correct step. Still want to make sure I'm reading things right.
Actually, disk->mirror is a bug here as we setup/fill it only after the
job is started. Passing mirror is the only correct thign to do.
The comment only states that the namespace/label/cgroup setup makes
sense only if the image has a backing chain.
>
> > + goto endjob;
> > + } else {
> > + if (qemuDomainDiskChainElementPrepare(driver, vm, mirror, false, true) < 0) {
> > + qemuDomainDiskChainElementRevoke(driver, vm, mirror);
> > + goto endjob;
> > + }
> > }
> >
> > if (!(job = qemuBlockJobDiskNew(disk, QEMU_BLOCKJOB_TYPE_COPY, device)))
> > diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> > index fb596d960f..c9e68397b6 100644
> > --- a/src/qemu/qemu_process.c
> > +++ b/src/qemu/qemu_process.c
> > @@ -7857,6 +7857,7 @@ qemuProcessRefreshLegacyBlockjob(void *payload,
> > virDomainDiskDefPtr disk;
> > qemuBlockJobDataPtr job;
> > qemuBlockJobType jobtype = info->type;
> > + qemuDomainObjPrivatePtr priv = vm->privateData;
> >
> > if (!(disk = qemuProcessFindDomainDiskByAliasOrQOM(vm, jobname, jobname))) {
> > VIR_DEBUG("could not find disk for block job '%s'", jobname);
> > @@ -7878,8 +7879,29 @@ qemuProcessRefreshLegacyBlockjob(void *payload,
> > disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_STATE_READY;
> > job->state = VIR_DOMAIN_BLOCK_JOB_READY;
> > }
> > +
> > + /* Pre-blockdev block copy labelled the chain of the mirrored device
> > + * just before pivoting. At that point it was no longer known whether
> > + * it's even necessary (e.g. disk is being reused). This code fixes
> > + * the labelling in case the job was started in a libvirt version
> > + * which did not label the chain when the block copy is being started.
> > + * Note that we can't do much on failure. */
> > + if (disk->mirrorJob == VIR_DOMAIN_BLOCK_JOB_TYPE_COPY) {
> > + if (qemuDomainDetermineDiskChain(priv->driver, vm, disk,
> > + disk->mirror, true) < 0)
> > + goto cleanup;
> > +
> > + if (disk->mirror->format &&
> > + disk->mirror->format != VIR_STORAGE_FILE_RAW &&
> > + (qemuDomainNamespaceSetupDisk(vm, disk->mirror) < 0 ||
>
> This is essentially the former qemuDomainBlockPivot right?
No. qemuDomainBlockPivot makes qemu switch to the new image. This just
sets up the labelling/namespaces/cgroups correctly for it to succeed
once the pivot operation is attempted, but that's still separate and
user controlled.
As we want to do the labeling in advance now, we need to fix the jobs
where it was not done before.
>
> Lots of voodoo black magic taking place.
>
> Things seem right, so consider it... If you think a few more words of
> comments may help, then great...
>
> Reviewed-by: John Ferlan <jferlan@xxxxxxxxxx>
>
> John
>
> > + qemuSetupImageChainCgroup(vm, disk->mirror) < 0 ||
> > + qemuSecuritySetImageLabel(priv->driver, vm, disk->mirror,
> > + true) < 0))
> > + goto cleanup;
> > + }
> > }
> >
> > + cleanup:
> > qemuBlockJobStartupFinalize(job);
> >
> > return 0;
> >
>
> --
> libvir-list mailing list
> libvir-list@xxxxxxxxxx
> https://www.redhat.com/mailman/listinfo/libvir-list
Attachment:
signature.asc
Description: PGP signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
