On Sep 03, 2021 / 18:58, Jens Axboe wrote:
> On 9/3/21 3:38 PM, Shinichiro Kawasaki wrote:
> > On Sep 03, 2021 / 06:38, Jens Axboe wrote:
> >> On 9/3/21 2:37 AM, Shin'ichiro Kawasaki wrote:
> >>> diff --git a/options.c b/options.c
> >>> index 8c2ab7cc..3fe1e6bd 100644
> >>> --- a/options.c
> >>> +++ b/options.c
> >>> @@ -3688,6 +3688,20 @@ struct fio_option fio_options[FIO_MAX_OPTS] = {
> >>> },
> >>> .parent = "thinktime",
> >>> },
> >>> + {
> >>> + .name = "thinktime_iotime",
> >>> + .lname = "Thinktime interval",
> >>> + .type = FIO_OPT_INT,
> >>> + .off1 = offsetof(struct thread_options, thinktime_iotime),
> >>> + .help = "IO time interval between 'thinktime'",
> >>> + .is_time = 1,
> >>> + .is_seconds = 1,
> >>> + .def = "0",
> >>> + .parent = "thinktime",
> >>> + .hide = 1,
> >>> + .category = FIO_OPT_C_IO,
> >>> + .group = FIO_OPT_G_THINKTIME,
> >>> + },
> >>
> >> Please line these up nicely as is done elsewhere.
> >
> > Ah, the order of the members are diffferent from others. I will move a
> > few members to make the order same as others.
>
> I didn't notice the ordering, but just to be clear, I'm just referring
> to the tabbing so the '=' line up. But if they are out of order, please
> do correct that too.
Thanks for the clarification. I will fix the order. As for tabbing, I found that
'=' is not lined up in existing members of fio_options[] either. Here I quote an
example from "startdelay" option.
{
.name = "startdelay",
.lname = "Start delay",
.type = FIO_OPT_STR_VAL_TIME,
.off1 = offsetof(struct thread_options, start_delay),
.off2 = offsetof(struct thread_options, start_delay_high),
.help = "Only start job when this period has passed",
.def = "0",
.is_seconds = 1,
.is_time = 1,
.category = FIO_OPT_C_GENERAL,
.group = FIO_OPT_G_RUNTIME,
},
The number of tab in front of '=' is single for all members. On the other hand,
some of the members have too long name: such as "is_time", "is_seconds" or
"category". So '=' can not be lined up for them. I think it would be the better
to allow unaligned '=' style for new options if they have the long name members.
After fixing the order of the members of the new option, that part will look as
follows:
+ .name = "thinktime_iotime",
+ .lname = "Thinktime interval",
+ .type = FIO_OPT_INT,
+ .off1 = offsetof(struct thread_options, thinktime_iotime),
+ .help = "IO time interval between 'thinktime'",
+ .def = "0",
+ .parent = "thinktime",
+ .hide = 1,
+ .is_seconds = 1,
+ .is_time = 1,
+ .category = FIO_OPT_C_IO,
+ .group = FIO_OPT_G_THINKTIME,
The unaligned '='s will be gathered at the last part, so I wish it does not look
so messy.
>
> >> Apart from that, this looks fine. Could really use an example job
> >> though, and an improvement in the explanation. The commit message looks
> >> great, but then the doc additions are pretty lackluster. Maybe steal a
> >> bit of the text you wrote from the commit message and put it in the
> >> shipped documentation?
> >
> > I will improve the descriptions of the new option in fio.1 and HOWTO,
> > copying some of the commit message. Also I will add "for example"
> > option usage in the descriptions.
>
> Sounds good, thanks!
>
> --
> Jens Axboe
>
--
Best Regards,
Shin'ichiro Kawasaki
