| Message ID | 20191122074826.1373-4-tao3.xu@intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | Build ACPI Heterogeneous Memory Attribute Table (HMAT) | expand |
Tao Xu <tao3.xu@intel.com> writes: > Add do_strtomul() to convert string according to different suffixes. > > Reviewed-by: Eduardo Habkost <ehabkost@redhat.com> > Signed-off-by: Tao Xu <tao3.xu@intel.com> What's the actual change here? "Refactor" suggests the interfaces stay the same, only their implementation changes. "Support suffixes list" suggests some interface supports something new.
On Mon, Nov 25, 2019 at 08:20:23AM +0100, Markus Armbruster wrote: > Tao Xu <tao3.xu@intel.com> writes: > > > Add do_strtomul() to convert string according to different suffixes. > > > > Reviewed-by: Eduardo Habkost <ehabkost@redhat.com> > > Signed-off-by: Tao Xu <tao3.xu@intel.com> > > What's the actual change here? "Refactor" suggests the interfaces stay > the same, only their implementation changes. "Support suffixes list" > suggests some interface supports something new. 1) Parameters added to suffix_mul() (suffix table); 2) do_strtomul() is being extracted from do_strtosz(). do_strtomul() interface and behavior stays the same.
Eduardo Habkost <ehabkost@redhat.com> writes: > On Mon, Nov 25, 2019 at 08:20:23AM +0100, Markus Armbruster wrote: >> Tao Xu <tao3.xu@intel.com> writes: >> >> > Add do_strtomul() to convert string according to different suffixes. >> > >> > Reviewed-by: Eduardo Habkost <ehabkost@redhat.com> >> > Signed-off-by: Tao Xu <tao3.xu@intel.com> >> >> What's the actual change here? "Refactor" suggests the interfaces stay >> the same, only their implementation changes. "Support suffixes list" >> suggests some interface supports something new. > > 1) Parameters added to suffix_mul() (suffix table); > 2) do_strtomul() is being extracted from do_strtosz(). > > do_strtomul() interface and behavior stays the same. Alright, it's two related changes squashed together (which tends to complicate writing good commit messages). 2) is really a refactoring. 1) is not: it makes suffix_mul() more flexible. Summarizing 1) and 2) as "refactor do_strtosz() to support suffixes list" is confusing, because it's about 1), while the interesting part is actually 2). Moreover, the commit message should state why these two changes are useful. It tries, but "to support suffixes list" merely kicks the can down the road, because the reader is left to wonder why supporting suffix lists is useful. It's actually for use by the next patch. So say that. I'll review the actual patch now.
Tao Xu <tao3.xu@intel.com> writes: > Add do_strtomul() to convert string according to different suffixes. > > Reviewed-by: Eduardo Habkost <ehabkost@redhat.com> > Signed-off-by: Tao Xu <tao3.xu@intel.com> > --- > > No changes in v17. > > Changes in v15: > - Add a new patch to refactor do_strtosz() (Eduardo) > --- > util/cutils.c | 72 ++++++++++++++++++++++++++++++--------------------- > 1 file changed, 42 insertions(+), 30 deletions(-) > > diff --git a/util/cutils.c b/util/cutils.c > index d94a468954..ffef92338a 100644 > --- a/util/cutils.c > +++ b/util/cutils.c > @@ -181,41 +181,37 @@ int fcntl_setfl(int fd, int flag) > } > #endif > > -static int64_t suffix_mul(char suffix, int64_t unit) > +static int64_t suffix_mul(const char *suffixes[], int num_suffix, > + const char *endptr, int *offset, int64_t unit) This function could really use a contract comment now. > { > - switch (qemu_toupper(suffix)) { > - case 'B': > - return 1; > - case 'K': > - return unit; > - case 'M': > - return unit * unit; > - case 'G': > - return unit * unit * unit; > - case 'T': > - return unit * unit * unit * unit; > - case 'P': > - return unit * unit * unit * unit * unit; > - case 'E': > - return unit * unit * unit * unit * unit * unit; > + int i, suffix_len; > + int64_t mul = 1; > + > + for (i = 0; i < num_suffix; i++) { Aha: @num_suffix is the number of elements in suffixes[]. I'd put a terminating NULL into suffixes[] and dispense with @num_suffix: for (i = 0; suffix[i]; i++) { > + suffix_len = strlen(suffixes[i]); @suffix_len should be size_t, because that's what strlen() returns. > + if (g_ascii_strncasecmp(suffixes[i], endptr, suffix_len) == 0) { @endptr is a terrible name: it points to the *beginning* of the suffix. > + *offset = suffix_len; @offset is a terrible name: it provides no clue whatsoever on its meaning. It's actually for receiving the length of the suffix. Please replace it by char **end, because that's how the related functions work. > + return mul; > + } > + mul *= unit; > } > + > return -1; > } > > /* > - * Convert string to bytes, allowing either B/b for bytes, K/k for KB, > - * M/m for MB, G/g for GB or T/t for TB. End pointer will be returned > - * in *end, if not NULL. Return -ERANGE on overflow, and -EINVAL on > - * other error. > + * Convert string according to different suffixes. End pointer will be returned > + * in *end, if not NULL. Return -ERANGE on overflow, and -EINVAL on other error. > */ > -static int do_strtosz(const char *nptr, const char **end, > - const char default_suffix, int64_t unit, > +static int do_strtomul(const char *nptr, const char **end, > + const char *suffixes[], int num_suffix, > + const char *default_suffix, int64_t unit, > uint64_t *result) The function comment is barely adequate before your patch: it doesn't explain the arguments. Your patch adds more arguments, thus more guesswork for the reader. Again, I'd put a terminating NULL into suffixes[] and dispense with @num_suffix. > { > int retval; > const char *endptr; > - unsigned char c; > int mul_required = 0; > + int offset = 0; > long double val, mul, integral, fraction; > > retval = qemu_strtold_finite(nptr, &endptr, &val); > @@ -226,12 +222,12 @@ static int do_strtosz(const char *nptr, const char **end, > if (fraction != 0) { > mul_required = 1; > } > - c = *endptr; > - mul = suffix_mul(c, unit); > + > + mul = suffix_mul(suffixes, num_suffix, endptr, &offset, unit); @endptr points behind the number, i.e. to the suffix, and @offset is the length of the suffix. Suggest to rename @endptr to @suffix, and replace @offset by @endptr. > if (mul >= 0) { > - endptr++; > + endptr += offset; > } else { > - mul = suffix_mul(default_suffix, unit); > + mul = suffix_mul(suffixes, num_suffix, default_suffix, &offset, unit); > assert(mul >= 0); Please assert "no trailing crap in @default_suffix". > } > if (mul == 1 && mul_required) { > @@ -256,19 +252,35 @@ out: > return retval; > } > > +/* > + * Convert string to bytes, allowing either B/b for bytes, K/k for KB, > + * M/m for MB, G/g for GB or T/t for TB. End pointer will be returned > + * in *end, if not NULL. Return -ERANGE on overflow, and -EINVAL on > + * other error. > + */ > +static int do_strtosz(const char *nptr, const char **end, > + const char *default_suffix, int64_t unit, > + uint64_t *result) > +{ > + static const char *suffixes[] = { "B", "K", "M", "G", "T", "P", "E" }; > + > + return do_strtomul(nptr, end, suffixes, ARRAY_SIZE(suffixes), > + default_suffix, unit, result); > +} > + > int qemu_strtosz(const char *nptr, const char **end, uint64_t *result) > { > - return do_strtosz(nptr, end, 'B', 1024, result); > + return do_strtosz(nptr, end, "B", 1024, result); > } > > int qemu_strtosz_MiB(const char *nptr, const char **end, uint64_t *result) > { > - return do_strtosz(nptr, end, 'M', 1024, result); > + return do_strtosz(nptr, end, "M", 1024, result); > } > > int qemu_strtosz_metric(const char *nptr, const char **end, uint64_t *result) > { > - return do_strtosz(nptr, end, 'B', 1000, result); > + return do_strtosz(nptr, end, "B", 1000, result); > } > > /**
On Tue, Nov 26, 2019 at 11:04:41AM +0100, Markus Armbruster wrote: > Eduardo Habkost <ehabkost@redhat.com> writes: > > > On Mon, Nov 25, 2019 at 08:20:23AM +0100, Markus Armbruster wrote: > >> Tao Xu <tao3.xu@intel.com> writes: > >> > >> > Add do_strtomul() to convert string according to different suffixes. > >> > > >> > Reviewed-by: Eduardo Habkost <ehabkost@redhat.com> > >> > Signed-off-by: Tao Xu <tao3.xu@intel.com> > >> > >> What's the actual change here? "Refactor" suggests the interfaces stay > >> the same, only their implementation changes. "Support suffixes list" > >> suggests some interface supports something new. > > > > 1) Parameters added to suffix_mul() (suffix table); > > 2) do_strtomul() is being extracted from do_strtosz(). > > > > do_strtomul() interface and behavior stays the same. > > Alright, it's two related changes squashed together (which tends to > complicate writing good commit messages). 2) is really a refactoring. > 1) is not: it makes suffix_mul() more flexible. Summarizing 1) and 2) > as "refactor do_strtosz() to support suffixes list" is confusing, > because it's about 1), while the interesting part is actually 2). > > Moreover, the commit message should state why these two changes are > useful. It tries, but "to support suffixes list" merely kicks the can > down the road, because the reader is left to wonder why supporting > suffix lists is useful. It's actually for use by the next patch. So > say that. Test case additions to test-cutils.c would go a long way to illustrating what is added & that its working correctly. Regards, Daniel
Daniel P. Berrangé <berrange@redhat.com> writes: > On Tue, Nov 26, 2019 at 11:04:41AM +0100, Markus Armbruster wrote: >> Eduardo Habkost <ehabkost@redhat.com> writes: >> >> > On Mon, Nov 25, 2019 at 08:20:23AM +0100, Markus Armbruster wrote: >> >> Tao Xu <tao3.xu@intel.com> writes: >> >> >> >> > Add do_strtomul() to convert string according to different suffixes. >> >> > >> >> > Reviewed-by: Eduardo Habkost <ehabkost@redhat.com> >> >> > Signed-off-by: Tao Xu <tao3.xu@intel.com> >> >> >> >> What's the actual change here? "Refactor" suggests the interfaces stay >> >> the same, only their implementation changes. "Support suffixes list" >> >> suggests some interface supports something new. >> > >> > 1) Parameters added to suffix_mul() (suffix table); >> > 2) do_strtomul() is being extracted from do_strtosz(). >> > >> > do_strtomul() interface and behavior stays the same. >> >> Alright, it's two related changes squashed together (which tends to >> complicate writing good commit messages). 2) is really a refactoring. >> 1) is not: it makes suffix_mul() more flexible. Summarizing 1) and 2) >> as "refactor do_strtosz() to support suffixes list" is confusing, >> because it's about 1), while the interesting part is actually 2). >> >> Moreover, the commit message should state why these two changes are >> useful. It tries, but "to support suffixes list" merely kicks the can >> down the road, because the reader is left to wonder why supporting >> suffix lists is useful. It's actually for use by the next patch. So >> say that. > > Test case additions to test-cutils.c would go a long way to illustrating > what is added & that its working correctly. Since this patch only prepares for new features, it doesn't need test updates. The next patch adds features, and it does update the tests.
On Tue, Nov 26, 2019 at 11:04:41AM +0100, Markus Armbruster wrote: > Eduardo Habkost <ehabkost@redhat.com> writes: > > > On Mon, Nov 25, 2019 at 08:20:23AM +0100, Markus Armbruster wrote: > >> Tao Xu <tao3.xu@intel.com> writes: > >> > >> > Add do_strtomul() to convert string according to different suffixes. > >> > > >> > Reviewed-by: Eduardo Habkost <ehabkost@redhat.com> > >> > Signed-off-by: Tao Xu <tao3.xu@intel.com> > >> > >> What's the actual change here? "Refactor" suggests the interfaces stay > >> the same, only their implementation changes. "Support suffixes list" > >> suggests some interface supports something new. > > > > 1) Parameters added to suffix_mul() (suffix table); > > 2) do_strtomul() is being extracted from do_strtosz(). > > > > do_strtomul() interface and behavior stays the same. > > Alright, it's two related changes squashed together (which tends to > complicate writing good commit messages). 2) is really a refactoring. > 1) is not: it makes suffix_mul() more flexible. Summarizing 1) and 2) > as "refactor do_strtosz() to support suffixes list" is confusing, > because it's about 1), while the interesting part is actually 2). I agree the interesting part is 2. I still consider 1 being refactoring, as it doesn't change any external behavior. > > Moreover, the commit message should state why these two changes are > useful. It tries, but "to support suffixes list" merely kicks the can > down the road, because the reader is left to wonder why supporting > suffix lists is useful. It's actually for use by the next patch. So > say that. Agreed. > > I'll review the actual patch now. Thanks!
diff --git a/util/cutils.c b/util/cutils.c index d94a468954..ffef92338a 100644 --- a/util/cutils.c +++ b/util/cutils.c @@ -181,41 +181,37 @@ int fcntl_setfl(int fd, int flag) } #endif -static int64_t suffix_mul(char suffix, int64_t unit) +static int64_t suffix_mul(const char *suffixes[], int num_suffix, + const char *endptr, int *offset, int64_t unit) { - switch (qemu_toupper(suffix)) { - case 'B': - return 1; - case 'K': - return unit; - case 'M': - return unit * unit; - case 'G': - return unit * unit * unit; - case 'T': - return unit * unit * unit * unit; - case 'P': - return unit * unit * unit * unit * unit; - case 'E': - return unit * unit * unit * unit * unit * unit; + int i, suffix_len; + int64_t mul = 1; + + for (i = 0; i < num_suffix; i++) { + suffix_len = strlen(suffixes[i]); + if (g_ascii_strncasecmp(suffixes[i], endptr, suffix_len) == 0) { + *offset = suffix_len; + return mul; + } + mul *= unit; } + return -1; } /* - * Convert string to bytes, allowing either B/b for bytes, K/k for KB, - * M/m for MB, G/g for GB or T/t for TB. End pointer will be returned - * in *end, if not NULL. Return -ERANGE on overflow, and -EINVAL on - * other error. + * Convert string according to different suffixes. End pointer will be returned + * in *end, if not NULL. Return -ERANGE on overflow, and -EINVAL on other error. */ -static int do_strtosz(const char *nptr, const char **end, - const char default_suffix, int64_t unit, +static int do_strtomul(const char *nptr, const char **end, + const char *suffixes[], int num_suffix, + const char *default_suffix, int64_t unit, uint64_t *result) { int retval; const char *endptr; - unsigned char c; int mul_required = 0; + int offset = 0; long double val, mul, integral, fraction; retval = qemu_strtold_finite(nptr, &endptr, &val); @@ -226,12 +222,12 @@ static int do_strtosz(const char *nptr, const char **end, if (fraction != 0) { mul_required = 1; } - c = *endptr; - mul = suffix_mul(c, unit); + + mul = suffix_mul(suffixes, num_suffix, endptr, &offset, unit); if (mul >= 0) { - endptr++; + endptr += offset; } else { - mul = suffix_mul(default_suffix, unit); + mul = suffix_mul(suffixes, num_suffix, default_suffix, &offset, unit); assert(mul >= 0); } if (mul == 1 && mul_required) { @@ -256,19 +252,35 @@ out: return retval; } +/* + * Convert string to bytes, allowing either B/b for bytes, K/k for KB, + * M/m for MB, G/g for GB or T/t for TB. End pointer will be returned + * in *end, if not NULL. Return -ERANGE on overflow, and -EINVAL on + * other error. + */ +static int do_strtosz(const char *nptr, const char **end, + const char *default_suffix, int64_t unit, + uint64_t *result) +{ + static const char *suffixes[] = { "B", "K", "M", "G", "T", "P", "E" }; + + return do_strtomul(nptr, end, suffixes, ARRAY_SIZE(suffixes), + default_suffix, unit, result); +} + int qemu_strtosz(const char *nptr, const char **end, uint64_t *result) { - return do_strtosz(nptr, end, 'B', 1024, result); + return do_strtosz(nptr, end, "B", 1024, result); } int qemu_strtosz_MiB(const char *nptr, const char **end, uint64_t *result) { - return do_strtosz(nptr, end, 'M', 1024, result); + return do_strtosz(nptr, end, "M", 1024, result); } int qemu_strtosz_metric(const char *nptr, const char **end, uint64_t *result) { - return do_strtosz(nptr, end, 'B', 1000, result); + return do_strtosz(nptr, end, "B", 1000, result); } /**