Re: [libvirt] [PATCHv4 14/18] blockjob: allow mirroring under SELinux
On 04/13/2012 02:45 PM, Jiri Denemark wrote:
On Mon, Apr 09, 2012 at 21:52:23 -0600, Eric Blake wrote:
> This copies heavily from qemuDomainSnapshotCreateSingleDiskActive(),
> in order to set the SELinux label, obtain locking manager lease, and
> audit the fact that we hand a new file over to qemu. Alas, releasing
> the lease and label on failure or at the end of the mirroring is a
> trickier prospect.
>
> +
> + if (virDomainLockDiskAttach(driver->lockManager, vm, disk) < 0)
> + goto endjob;
> + if (virSecurityManagerSetImageLabel(driver->securityManager, vm->def,
> + disk) < 0) {
> + if (virDomainLockDiskDetach(driver->lockManager, vm, disk) < 0)
> + VIR_WARN("Unable to release lock on %s", dest);
> + goto endjob;
> + }
Is there a reason for calling virDomainLockDiskDetach only when SetImageLabel
fails and not calling it if mirroring itself fails?
Simplicity of code - it's easier to code up permissions granting (and
leaking that) then it is to figure out which permissions need to be
reversed. I'll attempt to do a better job in v5, at least when
mirroring fails. But revoking permissions after 'drive-reopen' is a
bear - you can't do it immediately after the 'drive-reopen' command, but
have to wait until after the BLOCK_JOB_COMPLETED event before you know
that qemu no longer needs the file that you are about to revoke rights
on. And if libvirtd restarts in between the 'drive-reopen' command and
the eventual event from qemu, then we have to store state in the XML to
remind ourselves to do 'query-block' and 'query-block-job' when
reconnecting to the domain to figure out what state qemu is still in.
In fact, Paolo just made a proposal to upstream qemu today that would
add another aspect into the cleanup arena:
https://lists.gnu.org/archive/html/qemu-devel/2012-04/msg01638.html
the idea there is that libvirt would be responsible for creating a local
file (under /var/lib/libvirt/qemu, alongside the domain XML), and qemu
writes one byte to the file when the drive-reopen actually alters state,
so that part of the libvirtd restart recovery process is to check
whether the file is empty (the reopen didn't happen) or has contents
(the reopen succeeded, start the cleanup work for any resources no
longer appropriate to qemu). I'll have to figure out how much of that I
can interact with in my v5 series.
In short, I figured it was better to have code that could at least
demonstrate the API, even if it leaks, and work on plugging the leaks as
I have more time, than it was to wait for perfect code but miss out on
review and integration time.
--
Eric Blake eblake(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachments:
- signature.asc (application/pgp-signature — 620 bytes)