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 <[email protected]> >> --- > >> @@ -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 <[email protected]>
>
> 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 <[email protected]> (supporter:Block layer core)
Max Reitz <[email protected]> (supporter:Block layer core)
[email protected] (open list:Block layer core)
[email protected] (open list:All patches CC here)
$ scripts/get_maintainer.pl -f tests/qemu-iotests
Kevin Wolf <[email protected]> (supporter:Block layer core)
Max Reitz <[email protected]> (supporter:Block layer core)
[email protected] (open list:Block layer core)
[email protected] (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.)
signature.asc
Description: OpenPGP digital signature
