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

[John Snow <jsnow@redhat.com>]

On 8/20/19 1:24 PM, 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.
> 
>> @@ -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.
> 
> 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.
> 

Yes, this is a good idea. I'm sure over time we'll pick up uses of
pattern writing that will strengthen the the regression testing of the
feature, but for now a simple test case will help ensure it.

(It'll also help "document" how to use the feature for other test writers.)

Thanks!