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 <dplotni...@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 <ebl...@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!