parse: ignore leading minus sign when splitting options#2112
Conversation
| /* | ||
| * If the first character is a minus sign, it's | ||
| * likely a negative number, not a delimiter. | ||
| */ |
There was a problem hiding this comment.
Will - ever be a delimiter when we aren't parsing a FIO_OPT_RANGE? I think we can only be parsing {FIO_OPT_STR_VAL, FIO_OPT_STR_VAL_TIME, FIO_OPT_INT, FIO_OPT_ULL, FIO_OPT_BOOL, FIO_OPT_STR_SET, FIO_OPT_STR_VAL_ZONE} at this point...
There was a problem hiding this comment.
Yes, - is indeed used as a delimiter for those other types as well.
For example, options like bs, ba, or rate (which are evaluated as FIO_OPT_ULL or FIO_OPT_INT) allow users to specify different values for read, write, and trim. According to the fio documentation and the current parsing logic, these multiple values can be delimited by commas, colons, or hyphens.
Therefore, a command like fio --name=test --bs=4k-8k is perfectly valid and successfully assigns 4k to Read and 8k to Write by using the hyphen as a delimiter.
If we completely remove ptr2 = strchr(ptr, '-'); for types other than FIO_OPT_RANGE, it would break multi-value assignments like bs=4k-8k. That is why we still need to support - as a delimiter here, but merely skip it if it's the very first character (acting as a negative sign).
There was a problem hiding this comment.
Well this has turned out to be trickier than I first thought! From the documentation for "int":
If the option accepts an upper and lower range, use a colon ‘:’ or minus ‘-’ to separate such values. See irange.
If someone does bs=4k-8k I think they are confused because it looks like a range (so they should have used bsrange) but as you stated it will cause reads to be 4k and writes to be 8k. The docs for blocksize define it as taking a comma separated values and not a range. It's similar for blockalign and rate.
@vincentkfu I'm torn because the docs do describe this strange behaviour ("minus as a separator/delimiter") and taking it out will break backwards compatibility if someone has chosen to use it that way but isn't keeping it confusing? If we do say we keep this behaviour I see this PR as the only way to support negative numbers on things that aren't strings or floats.
There was a problem hiding this comment.
I don't see in the documentation where - should be used as a delimiter outside of places where a range is expected. I think treating - as a delimiter is just a quirk of the parsing logic. Ideally Fio would print an error message when it encounters a - when a comma is expected.
I'm inclined to allow the current undefined (as far as I could find) and confusing behavior to remain and think will take a closer look at the fix as time allows.
There was a problem hiding this comment.
I don't see in the documentation where - should be used as a delimiter outside of places where a range is expected.
Technically the docs describe this bizarre behaviour for things that take int parameter type. Perhaps we should change that text to leave the range stuff entirely to the irange description...
I'm inclined to allow the current undefined (as far as I could find) and confusing behavior to remain and think will take a closer look at the fix as time allows.
That seems fair.
|
@cenhuil One more request - please could we have a Real Name in front of your email address for both the commit author and Signed-off-by? |
When parsing an option like --nice=-20, the hyphen is incorrectly identified as a delimiter, splitting the value into two parts: an empty string and '20'. This results in a bogus out-of-range warning: 'nice: max value out of range: 20 (19 max)'. This commit fixes the issue by ignoring the hyphen if it appears as the very first character, correctly treating it as a negative sign rather than an option delimiter. Fixes axboe#1657 Signed-off-by: Huilin Cen <cenhuilin@kylinos.cn>
I have updated the Author and Signed-off-by fields to use my real name. Thanks! |
|
@vincentkfu this looks good to me. |
3 participants
When parsing an option like --nice=-20, the hyphen is incorrectly identified as a delimiter, splitting the value into two parts: an empty string and '20'. This results in a bogus out-of-range warning: 'nice: max value out of range: 20 (19 max)'.
This commit fixes the issue by ignoring the hyphen if it appears as the very first character, correctly treating it as a negative sign rather than an option delimiter.
Fixes #1657