On Wed, Apr 09, 2025 at 11:54:49AM -0700, Darrick J. Wong wrote:
> > + if (ioctl(fd, BLKRESETZONE, &range) < 0) {
> > + if (!quiet)
> > + printf(" FAILED\n");
>
> Should we print /why/ the zone reset failed?
As in the errno value? Sure.
> > +static int report_zones(const char *name, struct zone_info *zi)
> > +{
> > + struct blk_zone_report *rep;
> > + size_t rep_size;
> > + struct stat st;
> > + unsigned int i, n = 0;
> > + uint64_t device_size;
> > + uint64_t sector = 0;
> > + bool found_seq = false;
> > + int ret = 0;
> > + int fd;
>
> Nit: indenting
Fixed.
> > + goto out_close;
> > +
> > + if (ioctl(fd, BLKGETSIZE64, &device_size)) {
> > + ret = -EIO;
>
> ret = errno; ? But then...
>
> > + goto out_close;
> > + }
>
> ...what's the point in returning errors if the caller never checks?
Heh, I'll look into that.
> > + if (cli->xi->log.name && !cli->xi->log.isfile) {
> > + report_zones(cli->xi->log.name, &zt->log);
> > + if (zt->log.nr_zones) {
> > + fprintf(stderr,
> > +_("Zoned devices not supported as log device!\n"));
>
> Too bad, we really ought to be able to write logs to a zone device.
> But that's not in scope here.
That is on my todo list, but I need to finish support for the zoned RT
device first.
>
> > + usage();
> > + }
> > + }
> > +
> > + if (cli->rtstart) {
> > + if (cfg->rtstart) {
>
> Er... why are we checking the variable that we set four lines down?
> Is this supposed to be a check for external zoned rt devices?
>
> > + fprintf(stderr,
> > +_("rtstart override not allowed on zoned devices.\n"));
> > + usage();
> > + }
> > + cfg->rtstart = getnum(cli->rtstart, &ropts, R_START) / 512;
For devices with hardware zones rtstart is already set when we get
here and we don't want to allow overriding with the command line
parameter as that won't work.
> > +static void
> > +validate_rtgroup_geometry(
> > + struct mkfs_params *cfg)
> Hoisting this out probably should've been a separate patch.
Sure, I'll add a new one for the refactoring.
> <snip>
>
> > diff --git a/repair/agheader.c b/repair/agheader.c
> > index 5bb4e47e0c5b..048e6c3143b5 100644
> > --- a/repair/agheader.c
> > +++ b/repair/agheader.c
>
> Should this be in a different patch?
Yes.
