Use unsigned short type for exit_what and stonewall

[XeS0r]

Fixes: 64402a8 ("Expand choices for exitall")
Fixes: #1065
Signed-off-by: André Wild wild.andre.ae@gmail.com

[sitsofe]

@XeS0r Nice! Could you:

• Change the title so it prefixes the area (init: use unsigned short type for exit_what and stonewall)

And that's about all...

[sitsofe]

Actually your fix has uncovered a bug in

fio/examples/exitwhat.fio

Line 35 in 5c79c32

stonewall=1

- stonewall doesn't take any value (see https://fio.readthedocs.io/en/latest/fio_man.html#cmdoption-arg-stonewall ). Could you fix that up too?

[sitsofe]

The more I think about it the more I wonder - wouldn't it be better just to convert both these values (exit_what, stonewall) to be ints rather than shorts even if they could be smaller (even FIO_BOOL seems to go into an int)? numa_mem_mode gets away with it because it's not a fully fledged option passed in via the command line...

[XeS0r]

Actually your fix has uncovered a bug in

fio/examples/exitwhat.fio

Line 35 in 5c79c32

stonewall=1

• stonewall doesn't take any value (see https://fio.readthedocs.io/en/latest/fio_man.html#cmdoption-arg-stonewall ). Could you fix that up too?

I already fixed this issue. But I want to make sure that I don't find anything else. Now that I'm aware of the unit tests.

[XeS0r]

The more I think about it the more I wonder - wouldn't it be better just to convert both these values (exit_what, stonewall) to be ints rather than shorts even if they could be smaller (even FIO_BOOL seems to go into an int)? numa_mem_mode gets away with it because it's not a fully fledged option passed in via the command line...

I can also change them both to int if you prefer that solution.

[sitsofe]

If it looks like it would be smaller that would be the way to go (we would also likely need to bump FIO_VER FIO_SERVER_VER in https://github.com/axboe/fio/blob/master/server.h#L51 )...

[axboe]

Yeah just make them ints, and increment the FIO_SERVER_VER.

[sitsofe]

@XeS0r Looks good:

• Can you add a prefix to the commit message (see io_u: fix exit failure case when using rates and timeout #1050 (comment) for tips on the "house style")
• Could you fix up the incorrect test file

  fio/examples/exitwhat.fio

  Line 35 in 5c79c32

  stonewall=1

  ?

[sitsofe]

Ah one more:

• I think you need to fix up the packed structure too -

  fio/thread_options.h

  Line 497 in 5c79c32

  uint16_t exit_what;

[XeS0r]

@XeS0r Looks good:

* Can you add a prefix to the commit message (see [#1050 (comment)](https://github.com/axboe/fio/pull/1050#issuecomment-663606909) for tips on the "house style")

Yes sure, hopefully this prefix is fulfilling your requirements.

* Could you fix up the incorrect test file https://github.com/axboe/fio/blob/5c79c32f6afb39fae910912475e8fea786c3353e/examples/exitwhat.fio#L35
   ?

I've also fixed the comment.

[axboe]

If this change in job files is required, then we'll have to fix it so that's not the case. If it's not required, don't make the change in the job file.

[XeS0r]

Stonewalls usage is not documented like this. I've changed the man page and also fixed some other stuff in the example. Now it's up to you if we should use stonewall=1 in the example or not. Both will work and it's not required to set a value. But all other examples just use stonewall without any value assigned.

[axboe]

Looks good to me now, except you still have the exitwhat.fio change in there. Please get rid of that.

[axboe]

Not saying it's incorrect, but it doesn't matter and it's not related. You can do that as a separate commit, if you'd like. Ditto the fio.1 change, that should be a separate commit as well.

[axboe]

Ah screw it, I'll just pull it as it is, not worth changing it for imho.

[axboe]

Thanks for fixing this issue!

[XeS0r]

I would have changed that too. Was not sure If you want separate commits. Next time I'm aware of that. Thanks for your patience.