From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=44216 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1Q6hKM-0006Oc-Nw for qemu-devel@nongnu.org; Mon, 04 Apr 2011 06:48:13 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Q6hKJ-0000Om-Us for qemu-devel@nongnu.org; Mon, 04 Apr 2011 06:48:09 -0400 Received: from mx1.redhat.com ([209.132.183.28]:59680) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Q6hKJ-0000OI-Hd for qemu-devel@nongnu.org; Mon, 04 Apr 2011 06:48:07 -0400 Date: Mon, 4 Apr 2011 11:47:53 +0100 From: "Daniel P. Berrange" Subject: Re: [libvirt] [Qemu-devel] [PATCH v2 3/3] raw-posix: Re-open host CD-ROM after media change Message-ID: <20110404104753.GX13616@redhat.com> References: <1301425482-8722-1-git-send-email-stefanha@linux.vnet.ibm.com> <1301425482-8722-4-git-send-email-stefanha@linux.vnet.ibm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: Content-Transfer-Encoding: quoted-printable Reply-To: "Daniel P. Berrange" List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefan Hajnoczi Cc: Kevin Wolf , Anthony Liguori , Stefan Hajnoczi , Juan Quintela , libvir-list@redhat.com, qemu-devel@nongnu.org, Blue Swirl On Sun, Apr 03, 2011 at 07:06:17PM +0100, Stefan Hajnoczi wrote: > On Sun, Apr 3, 2011 at 2:12 PM, Blue Swirl wrote= : > > On Sun, Apr 3, 2011 at 2:57 PM, Stefan Hajnoczi = wrote: > >> On Tue, Mar 29, 2011 at 8:04 PM, Stefan Hajnoczi > >> wrote: > >>> Piggy-back on the guest CD-ROM polling to poll on the host. =C2=A0O= pen and > >>> close the host CD-ROM file descriptor to ensure we read the new siz= e and > >>> not a stale size. > >>> > >>> Two things are going on here: > >>> > >>> 1. If hald/udisks is not already polling CD-ROMs on the host then > >>> =C2=A0 re-opening the CD-ROM causes the host to read the new medium= 's size. > >>> > >>> 2. There is a bug in Linux which means the CD-ROM file descriptor m= ust > >>> =C2=A0 be re-opened in order for lseek(2) to see the new size. =C2=A0= The > >>> =C2=A0 inode size gets out of sync with the underlying device (whic= h you can > >>> =C2=A0 confirm by checking that /sys/block/sr0/size and lseek(2) do= not > >>> =C2=A0 match after media change). =C2=A0I have raised this with the > >>> =C2=A0 maintainers but we need a workaround for the foreseeable fut= ure. > >>> > >>> Note that these changes are all in a #ifdef __linux__ section. > >>> > >>> Signed-off-by: Stefan Hajnoczi > >>> --- > >>> =C2=A0block/raw-posix.c | =C2=A0 26 ++++++++++++++++++++++---- > >>> =C2=A01 files changed, 22 insertions(+), 4 deletions(-) > >>> > >>> diff --git a/block/raw-posix.c b/block/raw-posix.c > >>> index 6b72470..8b5205c 100644 > >>> --- a/block/raw-posix.c > >>> +++ b/block/raw-posix.c > >>> @@ -1238,10 +1238,28 @@ static int cdrom_is_inserted(BlockDriverSta= te *bs) > >>> =C2=A0 =C2=A0 BDRVRawState *s =3D bs->opaque; > >>> =C2=A0 =C2=A0 int ret; > >>> > >>> - =C2=A0 =C2=A0ret =3D ioctl(s->fd, CDROM_DRIVE_STATUS, CDSL_CURREN= T); > >>> - =C2=A0 =C2=A0if (ret =3D=3D CDS_DISC_OK) > >>> - =C2=A0 =C2=A0 =C2=A0 =C2=A0return 1; > >>> - =C2=A0 =C2=A0return 0; > >>> + =C2=A0 =C2=A0/* > >>> + =C2=A0 =C2=A0 * Close the file descriptor if no medium is present= and open it to poll > >>> + =C2=A0 =C2=A0 * again. =C2=A0This ensures the medium size is refr= eshed. =C2=A0If the file > >>> + =C2=A0 =C2=A0 * descriptor is kept open the size can become stale= . =C2=A0This is essentially > >>> + =C2=A0 =C2=A0 * replicating CD-ROM polling but is driven by the g= uest. =C2=A0As the guest > >>> + =C2=A0 =C2=A0 * polls, we poll the host. > >>> + =C2=A0 =C2=A0 */ > >>> + > >>> + =C2=A0 =C2=A0if (s->fd =3D=3D -1) { > >>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0s->fd =3D qemu_open(bs->filename, s->o= pen_flags, 0644); > >>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0if (s->fd < 0) { > >>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return 0; > >>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0} > >>> + =C2=A0 =C2=A0} > >>> + > >>> + =C2=A0 =C2=A0ret =3D (ioctl(s->fd, CDROM_DRIVE_STATUS, CDSL_CURRE= NT) =3D=3D CDS_DISC_OK); > >>> + > >>> + =C2=A0 =C2=A0if (!ret) { > >>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0close(s->fd); > >>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0s->fd =3D -1; > >>> + =C2=A0 =C2=A0} > >>> + =C2=A0 =C2=A0return ret; > >>> =C2=A0} > >>> > >>> =C2=A0static int cdrom_eject(BlockDriverState *bs, int eject_flag) > >>> -- > >>> 1.7.4.1 > >>> > >>> > >>> > >> > >> There is an issue with reopening host devices in QEMU when running > >> under libvirt. =C2=A0It appears that libvirt chowns image files (inc= luding > >> device nodes) so that the launched QEMU process can access them. > >> > >> Unfortunately after media change on host devices udev will reset the > >> ownership of the device node. =C2=A0This causes open(2) to fail with= EACCES > >> since the QEMU process does not have the right uid/gid/groups and > >> libvirt is unaware that the file's ownership has changed. > >> > >> In order for media change to work with Linux host CD-ROM it is > >> necessary to reopen the file (otherwise the inode size will not > >> refresh, this is an issue with existing kernels). > >> > >> How can libvirt's security model be made to support this case? =C2=A0= In > >> theory udev could be temporarily configured with libvirt permissions > >> for the CD-ROM device while passed through to the guest, but is that > >> feasible? > > > > How about something like this: Add an explicit reopen method to > > BlockDriver. Make a special block device for passed file descriptors. > > Pass descriptors in libvirt for CD-ROMs instead of the device paths. > > The reopen method for file descriptors should notify libvirt about > > need to pass a reopened descriptor and then block all accesses until = a > > new descriptor is available. This should also solve your earlier > > problem. >=20 > I'm hoping libvirt's behavior can be made to just work rather than > adding new features to QEMU. But perhaps passing file descriptors is > useful for more than just reopening host devices. This would > basically be a privilege separation model where the QEMU process isn't > able to open files itself but can request libvirt to open them on its > behalf. It is rather frickin' annoying the way udev resets the ownership when the media merely changes. If it isn't possible to stop udev doing this, then i think the only practical thing is to use ACLs instead of user/group ownership. We wanted to switch to ACLs in libvirt for other reasons already, but it isn't quite as simple as it sounds[1] so we've not done it just yet. Daniel [1] Mostly due to handling upgrades from existing libvirtd while VMs are running, and coping with filesystems which don't support ACLs (or have them turned of by mount options) --=20 |: http://berrange.com -o- http://www.flickr.com/photos/dberrange= / :| |: http://libvirt.org -o- http://virt-manager.or= g :| |: http://autobuild.org -o- http://search.cpan.org/~danberr= / :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vn= c :|