From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:54213) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1br0ry-0003qr-8k for qemu-devel@nongnu.org; Mon, 03 Oct 2016 06:53:18 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1br0rw-0005p2-IO for qemu-devel@nongnu.org; Mon, 03 Oct 2016 06:53:14 -0400 Date: Mon, 3 Oct 2016 11:52:59 +0100 From: "Daniel P. Berrange" Message-ID: <20161003105259.GF13491@redhat.com> Reply-To: "Daniel P. Berrange" References: <598de7ff27e32fcb1b7f677f40fb8da4f0a1f512.1475434971.git.tgolembi@redhat.com> <20161003085213.GA13491@redhat.com> <20161003124557.76781119@fiorina> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20161003124557.76781119@fiorina> Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH] raw-posix: add 'offset' and 'size' options List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: =?utf-8?B?VG9tw6HFoSBHb2xlbWJpb3Zza8O9?= Cc: qemu-block@nongnu.org, Kevin Wolf , qemu-devel@nongnu.org, Max Reitz On Mon, Oct 03, 2016 at 12:45:57PM +0200, Tom=C3=A1=C5=A1 Golembiovsk=C3=BD= wrote: > On Mon, 3 Oct 2016 09:52:13 +0100 > "Daniel P. Berrange" wrote: >=20 > > On Sun, Oct 02, 2016 at 09:13:29PM +0200, Tom=C3=A1=C5=A1 Golembiovsk= =C3=BD wrote: > > > Added two new options 'offset' and 'size'. This makes it possible t= o use > > > only part of the file as a device. This can be used e.g. to limit t= he > > > access only to single partition in a disk image or use a disk insid= e a > > > tar archive (like OVA). > > >=20 > > > For now this is only possible for files in read-only mode. It shoul= d be > > > possible to extend it later to allow read-write mode, but would pro= bably > > > require that the size of the device is kept constant (i.e. no resiz= ing). > > >=20 > > > Signed-off-by: Tom=C3=A1=C5=A1 Golembiovsk=C3=BD > > > --- > > > block/raw-posix.c | 97 +++++++++++++++++++++++++++++++++++++++++++= ++++++++---- > > > 1 file changed, 91 insertions(+), 6 deletions(-) =20 > >=20 > > An equivalent change is needed to raw-win32.c >=20 > That's what I feared. >=20 >=20 > > >=20 > > > diff --git a/block/raw-posix.c b/block/raw-posix.c > > > index 6ed7547..36f2712 100644 > > > --- a/block/raw-posix.c > > > +++ b/block/raw-posix.c =20 > >=20 > > > @@ -421,6 +435,17 @@ static int raw_open_common(BlockDriverState *b= s, QDict *options, > > > goto fail; > > > } > > > =20 > > > + s->offset =3D qemu_opt_get_size(opts, "offset", 0); > > > + s->assumed_size =3D qemu_opt_get_size(opts, BLOCK_OPT_SIZE, 0)= ; =20 > >=20 > > If the user didn't provide "size", then we should initialize > > it to the actual size of the underlying storage, rather than > > to 0. >=20 > I rather wanted to distinguish the case when no offset or size was > specified to be able to limit the scope of introduced changes. IMHO that's a mistake - it leads to 2 different codepaths one of which will be mostly unused & untested, so liable to bitrot. > > > + > > > + if (((bs->drv !=3D &bdrv_file) || !bs->read_only) && =20 > >=20 > > Why the check against bdrv_file ? >=20 > To limit it only to files. Maybe there is better way to do that? The > devices have a nasty habit to change the size. Sure, this can happen to > file too, e.g. if somebody truncates the file outside QEMU. But that's > rather a bad behaviour. For devices changing the size may be perfectly > valid operation, e.g. replacing CD in drive or card in a card reader. The raw driver is usable over any storage backend (file, rbd, iscsi, etc, etc) and it is valid to want to use a offset/size parameter in combination with any of them. So we should not restrict it to just files. > > > + ((s->offset > 0) || (s->assumed_size > 0))) { > > > + error_setg(errp, "offset and size options are allowed only= for " > > > + "files in read-only mode"); > > > + ret =3D -EINVAL; > > > + goto fail; > > > + } =20 > >=20 > > Why did you restrict it to read-only instead of adding the offset log= ic > > to the write & truncate methods. IMHO if we're going to add this feat= ure > > we should make it work in all scenarios, not just do 1/2 the job. >=20 > I still plan to do that, but I didn't feel confident enough to do it in > the first run. >=20 > We need to decide what is the correct behaviour here first. Since the > image we're working with is contained in some larger unit it cannot be > resized freely. There are two option that come to my mind: >=20 > 1) block truncate/grow completely, > 2) allow image to be truncated and grown only if the new size does not > go over the original size. >=20 > If we go with the second option then I have a another question. How > strict is (or should be) qemu about the sizes being block aligned? I'm > still little bit fuzzy on that. Somewhere it is assumed that the size i= s > multiple of 512, sometimes qemu automatically rounds up if it isn't and > sometimes it seems to me that the size can be arbitrary. We should not make assumptions about what is a valid or invalid usage, as QEMU doesn't have enough knowledge to do that correctly. IOW, we should just allow truncate with no restrictions. It is up to the calling app to figure out whether truncate makes sense or not. Regards, Daniel --=20 |: http://berrange.com -o- http://www.flickr.com/photos/dberrange= / :| |: http://libvirt.org -o- http://virt-manager.or= g :| |: http://entangle-photo.org -o- http://search.cpan.org/~danberr= / :|