Re: [Qemu-devel] [PATCH v2] Add 'offset' and 'size' options

From: Kevin Wolf <kwolf@redhat.com>
To: "Tomáš Golembiovský" <tgolembi@redhat.com>
Cc: qemu-devel@nongnu.org, Max Reitz <mreitz@redhat.com>,
	Markus Armbruster <armbru@redhat.com>,
	Eric Blake <eblake@redhat.com>,
	qemu-block@nongnu.org
Subject: Re: [Qemu-devel] [PATCH v2] Add 'offset' and 'size' options
Date: Tue, 18 Oct 2016 13:49:42 +0200	[thread overview]
Message-ID: <20161018114942.GE4706@noname.str.redhat.com> (raw)
In-Reply-To: <cover.1476741489.git.tgolembi@redhat.com>

Am 18.10.2016 um 00:25 hat Tomáš Golembiovský geschrieben:
> This is a follow-up to the patch:
>     [PATCH] raw-posix: add 'offset' and 'size' options
> 
> The main changes are:
>  -  options were moved from 'file' driver into 'raw' driver as suggested
>  -  added support for writing, reopen and truncate when possible
> 
> If I forgot to address somebody's comments feel free to raise them again,
> please.
> 
> Some general notes to the code:
> 
> 1)  The size is rounded *down* to the 512 byte boundary. It's not that
>     the raw driver really cares about this, but if we don't do it then 
>     bdrv_getlength() will do that instead of us. The problem is that
>     bdrv_getlength() does round *up* and this can lead to reads/writes
>     outside the specified 'size'.

I think it might be better to just check whether offset/size are
correctly aligned and error out if they aren't.

Then once we made the necessary changes to allow byte alignment (I think
what prevents it is mostly bs->total_sectors, right?) we can allow that
in the raw format driver, which will only make previously failing
options work rather than changing the behaviour of already working
configurations (we can't do the latter without a very good justification
because of compatibility).

> 2)  We don't provide '.bdrv_get_allocated_file_size' function. As a
>     result the information about allocated disk size reports size of the
>     whole file. This is, rather confusingly, larger than the provided
>     'size'. But I don't think this matters much. Note that we don't have
>     any easy way how to get the correct information here apart from
>     checking all the block with bdrv_co_get_block_status() (as suggested
>     by Kevin Wolf).
> 
> 3)  No options for raw_create(). The 'size' and 'offset' options were
>     added only to open/reopen. In my opinion there is no real reason for
>     them there. AFAIK you cannot create embeded QCOW2/VMDK/etc. image
>     that way anyway.

These two things are fine with me.

Kevin