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 <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.
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 <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.
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 <kw...@redhat.com> (supporter:Block layer core)
Max Reitz <mre...@redhat.com> (supporter:Block layer core)
qemu-bl...@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 <kw...@redhat.com> (supporter:Block layer core)
Max Reitz <mre...@redhat.com> (supporter:Block layer core)
qemu-bl...@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.)
signature.asc
Description: OpenPGP digital signature
