Re: [Qemu-block] [Qemu-devel] [PATCH v7] pflash: Require backend size to match device, improve errors

[Markus Armbruster]

Kevin Wolf <address@hidden> writes:

> Am 08.03.2019 um 15:29 hat Markus Armbruster geschrieben:
>> Kevin Wolf <address@hidden> writes:
>> 
>> > Am 08.03.2019 um 13:28 hat Markus Armbruster geschrieben:
>> >> Laszlo Ersek <address@hidden> writes:
>> >> > This one has got to be one of the longest bike-shedding sessions! :)
>> >> >
>> >> > I'm fine with this patch, but I could suggest two improvements.
>> >> >
>> >> > (1) When blk_getlength() fails, we could format the negative error code
>> >> > returned by it into the error message.
>> >> 
>> >> I can do that.
>> >
>> > By using error_setg_errno(), I assume. Not throwing away error details
>> > is always good.
>> >
>> >> > (2) We could extract the common code to a new function in
>> >> > "hw/block/block.c". (It says "Common code for block device models" on
>> >> > the tin.)
>> >> 
>> >> There's so much common code in these two files even before this patch...
>> >
>> > My understanding is that hw/block/block.c contains code that is
>> > potentially useful to all kinds of block devices, not random code that
>> > two specific similar devices happen to share.
>> >
>> > If we want to deduplicate some code in the flash devices, without any
>> > expectation that other devices will use it at some point, I'd rather
>> > create a new source file hw/block/pflash_common.c or something like
>> > that.
>> 
>> Yes.
>> 
>> The helper I came up with (appended) isn't really specific to flash
>> devices.  Would it be okay for hw/block/block.c even though only the two
>> flash devices use it for now?
>
> Hm, it feels more like a helper for devices that can't decide whether
> they want to be a block device or not. Or that actually don't want to be
> a block device, but use a BlockBackend anyway.

I think the latter's precisely what the two pflash devices are.

>                                                Reading in the whole
> image isn't something that a normal block device would do.

No argument.

> But yes, it doesn't have flash-specific knowledge, even though I hope
> that it's functionality that will remain very specific to these two
> devices.
>
> So it's your call, I don't have a strong opinion either way.

Understood.

>> bool blk_check_size_and_read_all(BlockBackend *blk, void *buf, hwaddr size,
>>                                  Error **errp)
>> {
>>     int64_t blk_len;
>>     int ret;
>> 
>>     blk_len = blk_getlength(blk);
>>     if (blk_len < 0) {
>>         error_setg_errno(errp, -blk_len,
>>                          "can't get size of block backend '%s'",
>>                          blk_name(blk));
>>         return false;
>>     }
>>     if (blk_len != size) {
>>         error_setg(errp, "device requires %" PRIu64 " bytes, "
>>                    "block backend '%s' provides %" PRIu64 " bytes",
>>                    size, blk_name(blk), blk_len);
>
> Should size use HWADDR_PRIu?

Yes.

> I'm not sure if printing the BlockBackend name is a good idea because
> hopefully one day the BlockBackend will be anonymous even for the flash
> devices.

Hmm.  Tell me what else I can use to identify the troublemaker to the
user.

For the second error, I could sidestep the issue and point to frontend
instead (oh, now I have an even thornier issue).  The first and third
error are *backend* errors, only related to the frontend since the
frontend happens to use them.  I could perhaps identify them as "backend
of frontend F".  If we want to do that going forward, we need suitable
helpers, so we don't reinvent this wheel in every block frontend.

>>         return false;
>>     }
>> 
>>     /* TODO for @size > BDRV_REQUEST_MAX_BYTES, we'd need to loop */
>>     assert(size <= BDRV_REQUEST_MAX_BYTES);
>
> I don't think we'd ever want to read in more than 2 GB into a memory
> buffer. Before we even get close to this point, the devices should be
> reworked to be more like an actual block device and read only what is
> actually accessed.

I'll steal from that to turn the comment into a proper excuse for my
lazy assertion.  Thanks!

>>     ret = blk_pread(blk, 0, buf, size);
>>     if (ret < 0) {
>>         error_setg_errno(errp, -ret, "can't read block backend '%s'",
>>                          blk_name(blk));
>>         return false;
>>     }
>>     return true;
>> }
>
> Kevin