Damien Le Moal <damien.lem...@opensource.wdc.com> 于2022年6月28日周二 17:11写道: > > On 6/28/22 16:56, Stefan Hajnoczi wrote: > > On Mon, Jun 27, 2022 at 08:19:14AM +0800, Sam Li wrote: > >> diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c > >> index 2f0d8ac25a..3f2592b9f5 100644 > >> --- a/qemu-io-cmds.c > >> +++ b/qemu-io-cmds.c > >> @@ -1706,6 +1706,122 @@ static const cmdinfo_t flush_cmd = { > >> .oneline = "flush all in-core file state to disk", > >> }; > >> > >> +static int zone_report_f(BlockBackend *blk, int argc, char **argv) > >> +{ > >> + int ret; > >> + int64_t offset, len, nr_zones; > >> + int i = 0; > >> + > >> + ++optind; > >> + offset = cvtnum(argv[optind]); > >> + ++optind; > >> + len = cvtnum(argv[optind]); > >> + ++optind; > >> + nr_zones = cvtnum(argv[optind]); > >> + > >> + g_autofree BlockZoneDescriptor *zones = g_new(BlockZoneDescriptor, > >> nr_zones); > >> + ret = blk_zone_report(blk, offset, len, &nr_zones, zones); > >> + while (i < nr_zones) { > > > > Does blk_zone_report() set nr_zones to 0 on failure or do we need to > > check if (ret < 0) here? > > ret = 0 means "no zone reported" which happen only if nr_zones is 0 or the > start offset is past the end of the disk capacity. ret < 0 would mean that > a report zone operation was actually attempted and failed (EIO, ENOMEM etc). > > > > >> + fprintf(stdout, "start: 0x%lx, len 0x%lx, cap 0x%lx, wptr 0x%lx, " > > > > The rest of the source file uses printf() instead of fprintf(stdout, > > ...). That's usually preferred because it's shorter. > > > >> + "zcond:%u, [type: %u]\n", > > > > Please use PRIx64 instead of lx format specifiers for portability. On > > 32-bit hosts lx is 32-bit, not 64-bit. You can grep QEMU's code for > > examples of PRIx64. > > > >> + zones[i].start, zones[i].length, zones[i].cap, > >> zones[i].wp, > >> + zones[i].cond, zones[i].type); > >> + ++i; > >> + } > > > > A for loop is more idiomatic: > > > > for (int i = 0; i < nr_zones; i++) { > > ... > > } > > > >> + return ret; > >> +} > >> + > >> +static const cmdinfo_t zone_report_cmd = { > >> + .name = "zone_report", > >> + .altname = "f", > >> + .cfunc = zone_report_f, > >> + .argmin = 3, > >> + .argmax = 3, > >> + .args = "offset [offset..] len [len..] number [num..]", > > > > The arguments are "offset len number". This command does not accept > > optional offset/len/num arguments. > > The arguments should be offset + len OR offset + number of zones. Having > the 3 of them does not make sense to me. The interface would then be: > > (1) offset + len -> report all zones in the block range [offset .. offset > + len - 1] > > (2) offset + number of zones -> report at most "number of zones" from the > zone containing the block at "offset". > > (2) matches the semantic used at the device command level. So I prefer to > approach (1). Yes, I'll remove the len argument then.
> > > > >> + .oneline = "report a number of zones", > > > > Maybe "report zone information". > > > >> +}; > >> + > >> +static int zone_open_f(BlockBackend *blk, int argc, char **argv) > >> +{ > >> + int64_t offset, len; > >> + ++optind; > >> + offset = cvtnum(argv[optind]); > >> + ++optind; > >> + len = cvtnum(argv[optind]); > >> + return blk_zone_mgmt(blk, zone_open, offset, len); > > > > Where is the error reported? When I look at read_f() I see: > > > > if (ret < 0) { > > printf("read failed: %s\n", strerror(-ret)); > > > > I think something similar is needed because qemu-io.c does not print an > > error message for us. The same is true for the other commands defined in > > this patch. > > > >> +} > >> + > >> +static const cmdinfo_t zone_open_cmd = { > >> + .name = "zone_open", > >> + .altname = "f", > >> + .cfunc = zone_open_f, > >> + .argmin = 2, > >> + .argmax = 2, > >> + .args = "offset [offset..] len [len..]", > > > > There are no optional offset/len args. The same is true for the other > > commands below. > > > -- > Damien Le Moal > Western Digital Research