Re: [Qemu-devel] [PATCH v10] qemu-io: add pattern file for write command

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1.1: Type: text/plain, Size: 4209 bytes --]

On 20.08.19 19:24, Eric Blake wrote:
> On 8/20/19 11:46 AM, Denis Plotnikov wrote:
>> The patch allows to provide a pattern file for write
>> command. There was no similar ability before.
>>
>> Signed-off-by: Denis Plotnikov <dplotnikov@virtuozzo.com>
>> ---
> 
>> @@ -983,8 +1057,9 @@ static int write_f(BlockBackend *blk, int argc, char **argv)
>>      /* Some compilers get confused and warn if this is not initialized.  */
>>      int64_t total = 0;
>>      int pattern = 0xcd;
>> +    const char *file_name = NULL;
>>  
>> -    while ((c = getopt(argc, argv, "bcCfnpP:quz")) != -1) {
>> +    while ((c = getopt(argc, argv, "bcCfnpP:quzs:")) != -1) {
> 
> This one looks odd (I would have preserved ordering by sticking s:
> between q and u).  But a maintainer could fix that.
> 
>>          switch (c) {
>>          case 'b':
>>              bflag = true;
>> @@ -1020,6 +1095,10 @@ static int write_f(BlockBackend *blk, int argc, char **argv)
>>          case 'z':
>>              zflag = true;
>>              break;
>> +        case 's':
>> +            sflag = true;
>> +            file_name = optarg;
>> +            break;
> 
> Likewise, sorting the cases in the same order as the getopt() listing
> helps in finding code during later edits.

But it is in order of the getopt() listing. ;-)

>> @@ -1088,7 +1168,14 @@ static int write_f(BlockBackend *blk, int argc, char **argv)
>>      }
>>  
>>      if (!zflag) {
>> -        buf = qemu_io_alloc(blk, count, pattern);
>> +        if (sflag) {
>> +            buf = qemu_io_alloc_from_file(blk, count, file_name);
>> +            if (!buf) {
>> +                return -EINVAL;
>> +            }
>> +        } else {
>> +            buf = qemu_io_alloc(blk, count, pattern);
>> +        }
> 
> Pre-existing, but it is odd that qemu_io_alloc() exit()s rather than
> returning NULL on huge allocation requests that can't be met.  (Then
> again, we have an early exit on any length > 2G, and 2G allocations tend
> to succeed on modern development machines).  Perhaps it would be nice to
> teach qemu-io to use blk_try_blockalign for more graceful handling even
> on 32-bit platforms, but that's not the problem of your patch.

Then again, this is qemu-io.  Printing an error instead of just aborting
doesn’t really help anyone.

Also, the code would be wrong without an early exit on a length >
INT_MAX.  (Because pattern_len is an int, so the result of fread() might
overflow otherwise, which would be bad.)

(I just noticed that fread() might do a short read, but let’s just
ignore this at this point.)

> Option ordering is minor enough that I'm fine giving:
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>
> 
> Now, to figure out which maintainer should take it.  Perhaps you want to
> add a patch 2/1 that adds an iotest using this new mode, to a) ensure it
> doesn't regress, and b) makes it reasonable to take in through the
> iotest tree.

Adding a test does not seem to bad of an idea, but I don’t see how that
would clarify things.  Both qemu-io and the iotests are part of the
block layer core:

$ scripts/get_maintainer.pl -f qemu-io-cmds.c
Kevin Wolf <kwolf@redhat.com> (supporter:Block layer core)
Max Reitz <mreitz@redhat.com> (supporter:Block layer core)
qemu-block@nongnu.org (open list:Block layer core)
qemu-devel@nongnu.org (open list:All patches CC here)

$ scripts/get_maintainer.pl -f tests/qemu-iotests
Kevin Wolf <kwolf@redhat.com> (supporter:Block layer core)
Max Reitz <mreitz@redhat.com> (supporter:Block layer core)
qemu-block@nongnu.org (open list:Block layer core)
qemu-devel@nongnu.org (open list:All patches CC here)


So we only need to figure out whether it should be Kevin or me to take
it; but Kevin is on PTO, so that decision is simple. :-)

Therefor, I’ve changed the optstring (and switch case) order to be
alphabetical, and applied the patch to my block branch:

https://git.xanclic.moe/XanClic/qemu/commits/branch/block

Thanks for the patch and the review,

Max


(I wouldn’t mind an iotest, but well.  qemu-io itself is a testing
utility, so I don’t deem it important to test it.)


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]