Re: [Qemu-devel] [RFC PATCH v6 2/4] hw/block: Pad undersized read-only images with 0xFF

[Markus Armbruster]

Kevin Wolf <address@hidden> writes:

> Am 07.03.2019 um 11:05 hat Markus Armbruster geschrieben:
>> Markus Armbruster <address@hidden> writes:
>> 
>> > From: Alex Bennée <address@hidden>
>> >
>> > We reject undersized images.  As of the previous commit, even with a
>> > decent error message.  Still, this is a potentially confusing
>> > stumbling block when you move from using -bios to using -drive
>> > if=pflash,file=blob,format=raw,readonly for loading your firmware
>> > code. To mitigate that we automatically pad in the read-only case and
>> > warn the user when we have performed magic to enable things to Just
>> > Work (tm).
>> >
>> > Signed-off-by: Alex Bennée <address@hidden>
>> > Reviewed-by: Laszlo Ersek <address@hidden>
>> > Signed-off-by: Markus Armbruster <address@hidden>
>> 
>> I think this convenience feature is a bad idea, and this patch should
>> not be applied.  Here are my reasons.
>> 
>> 1. As I explained in my disccussion of v5[*], there is no single true
>>    way to pad.  This patch pads with 0xFF.  When working with physical
>>    devices, you'd sometimes pad that way, but at other times, you'd pad
>>    differently.
>> 
>> 2. Except this patch doesn't *actually* pad with 0xFF.  The block layer
>>    silently pads with zeros up to the next multiple of 512.  Evidence:
>> 
>>     $ yes | dd of=4090b.img bs=1 count=4090
>>     4090+0 records in
>>     4090+0 records out
>>     4090 bytes (4.1 kB, 4.0 KiB) copied, 0.0186459 s, 219 kB/s
>>     $ qemu-io -f raw -c 'read -v 4000 96' 4090b.img
>>     00000fa0:  79 0a 79 0a 79 0a 79 0a 79 0a 79 0a 79 0a 79 0a  
>> y.y.y.y.y.y.y.y.
>>     00000fb0:  79 0a 79 0a 79 0a 79 0a 79 0a 79 0a 79 0a 79 0a  
>> y.y.y.y.y.y.y.y.
>>     00000fc0:  79 0a 79 0a 79 0a 79 0a 79 0a 79 0a 79 0a 79 0a  
>> y.y.y.y.y.y.y.y.
>>     00000fd0:  79 0a 79 0a 79 0a 79 0a 79 0a 79 0a 79 0a 79 0a  
>> y.y.y.y.y.y.y.y.
>>     00000fe0:  79 0a 79 0a 79 0a 79 0a 79 0a 79 0a 79 0a 79 0a  
>> y.y.y.y.y.y.y.y.
>>     00000ff0:  79 0a 79 0a 79 0a 79 0a 79 0a 00 00 00 00 00 00  
>> y.y.y.y.y.......
>>     read 96/96 bytes at offset 4000
>>     96 bytes, 1 ops; 0.0001 sec (694.444 KiB/sec and 7407.4074 ops/sec)

By the way, when you write to the padding, the block layer grows the
file to the next multiple of 512.

>> 
>>    This patch then pads some more with 0xFF to the flash memory size.
>> 
>>    Because of that the way the magic padding works makes no sense, to be
>>    frank.  Going back to v3's zero padding would at least be explainable
>>    without blushing.
>> 
>>    I consider the block layer's padding a misfeature here, but right now
>>    we got to play the hand we've been dealt.
>
> That will be solved as soon as the block layer is consistently converted
> to byte granularity. We've already converted a lot, but bdrv_getlength()
> is still sector (512 bytes) granularity.
>
> You could argue that file-posix should just reject files that are not
> sector aligned, but that's probably not right either because image
> formats don't necessarily have that alignment for their files.

Yes, it could well turn out to be overly restrictive.

> Maybe disk device should reject being attached to nodes that aren't a
> multiple of their physical and logical sector size. A 512-byte aligned
> image is probably suitable for most disks, but might not be for a virtual
> native 4k disk.

Makes sense to me.

Could perhaps be done in or near blkconf_blocksizes().

>> 3. Convenience magic has bitten us in the posterior so many times.  Just
>>    say no unless there's a really compelling use case.  Where's the use
>>    case for this one?  We've rejected undersized images for ages, and
>>    nobody complained.  Why add convenience magic now?
>
> I agree.

Okay.  Let's take just the error reporting improvment then (PATCH 1 and
its fixup).  Since we all seem to agree on upgrading the warning to an
error, do that.  Don't take the convenience feature now (PATCH 2 and its
fixup).  We can always revisit it later.

This should give us a good chance at getting it done in time for the
soft freeze.

I'll prepare v7.

Thanks!