Re: [Qemu-devel] [PATCH v8 20/21] vvfat: Switch to .bdrv_co_block_status()

[Kevin Wolf <kwolf@redhat.com>]

Am 14.02.2018 um 15:50 hat Eric Blake geschrieben:
> On 02/14/2018 07:12 AM, Kevin Wolf wrote:
> > Am 13.02.2018 um 21:27 hat Eric Blake geschrieben:
> > > We are gradually moving away from sector-based interfaces, towards
> > > byte-based.  Update the vvfat driver accordingly.  Note that we
> > > can rely on the block driver having already clamped limits to our
> > > block size, and simplify accordingly.
> > > 
> > > Signed-off-by: Eric Blake <eblake@redhat.com>
> > > Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> > > Reviewed-by: Fam Zheng <famz@redhat.com>
> 
> > >   {
> > > -    *n = bs->total_sectors - sector_num;
> > > -    if (*n > nb_sectors) {
> > > -        *n = nb_sectors;
> > > -    } else if (*n < 0) {
> > > -        return 0;
> > > -    }
> > > +    *n = bytes;
> > >       return BDRV_BLOCK_DATA;
> > >   }
> > 
> > Preexisting, but this is inconsistent with other protocol drivers as far
> > as OFFSET_VALID is concerned (as I hinted in another mail, I like it
> > better this way, but it's still inconsistent).
> > 
> > Do we actually need any callback here or could the solution be to simply
> > remove it like with nvme?
> 
> Does that mean io.c's defaults for protocol drivers is wrong?  It defaults
> to setting OFFSET_VALID and *map on all protocol drivers without a callback
> (at least nvme, nbd); I didn't delete this callback because I noticed the
> difference in return value, and couldn't justify whether it was intentional.
> Also, vvfat is weird - it is somewhat of a format driver, rather than just a
> protocol; even though it sets .protocol_name.  It may be possible for vvfat
> to actually set OFFSET_VALID to particular offsets within the host file that
> correspond to what the guest would read, where it is not a simple 1:1
> mapping, precisely because it is imposing format on the host file.  However,
> vvfat is one of those things that I try to avoid as much as possible,
> because it is just so weird.

As I said in my reply to the null block driver, OFFSET_VALID doesn't
really make sense for protocol drivers anyway. Making use of it with
vvfat isn't any more practical than directly accessing the undefined
data of the null driver.

I think the unwritten rule at the moment is that protocols should always
set OFFSET_VALID and *file = bs (even though it doesn't make sense). So
with the current interface, I'd consider deleting the callback a vvfat
fix.

I also think that we should possibly look into changing the interface so
that protocols don't set OFFSET_VALID and *file, but then the default
handling would change too, and deleting the callback in vvfat would
still be right.

As this is preexisting, I'm okay with just merging the series as it is,
and then we can handle this while dealing with the more fundamental
question what protocol drivers should return in general.

Kevin