| Message ID | 20181012114916.23532-2-david@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | qapi/range/memory-device: fixes and cleanups | expand |
Quick peek only for now. David Hildenbrand <david@redhat.com> writes: > Right now, we parse uint64_t values just like int64_t values, resulting > in negative values getting accepted and certain valid large numbers only > being representable as negative numbers. Also, reported errors indicate > that an int64_t is expected. > > Parse uin64_t separately. Implementation inspired by original > parse_str() implementation. > > E.g. we can now specify > -device nvdimm,memdev=mem1,id=nv1,addr=0xFFFFFFFFC0000000 > Instead of going via negative values > -device nvdimm,memdev=mem1,id=nv1,addr=-0x40000000 > > Resulting in the same values > > (qemu) info memory-devices > Memory device [nvdimm]: "nv1" > addr: 0xffffffffc0000000 > slot: 0 > node: 0 > > Signed-off-by: David Hildenbrand <david@redhat.com> Related work on the QObject input visitor: commit 5923f85fb82df7c8c60a89458a5ae856045e5ab1 Author: Marc-André Lureau <marcandre.lureau@redhat.com> Date: Wed Jun 7 20:36:03 2017 +0400 qapi: update the qobject visitor to use QNUM_U64 Switch to use QNum/uint where appropriate to remove i64 limitation. The input visitor will cast i64 input to u64 for compatibility reasons (existing json QMP client already use negative i64 for large u64, and expect an implicit cast in qemu). Note: before the patch, uint64_t values above INT64_MAX are sent over json QMP as negative values, e.g. UINT64_MAX is sent as -1. After the patch, they are sent unmodified. Clearly a bug fix, but we have to consider compatibility issues anyway. libvirt should cope fine, because its parsing of unsigned integers accepts negative values modulo 2^64. There's hope that other clients will, too. Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> Reviewed-by: Markus Armbruster <armbru@redhat.com> Message-Id: <20170607163635.17635-12-marcandre.lureau@redhat.com> [check_native_list() tweaked for consistency with signed case] Signed-off-by: Markus Armbruster <armbru@redhat.com> Note who it considers backward compatibility. Have you done that for the string input visitor? The commit message should tell. > --- > qapi/string-input-visitor.c | 117 ++++++++++++++++++++++++++++++++---- > 1 file changed, 106 insertions(+), 11 deletions(-) > > diff --git a/qapi/string-input-visitor.c b/qapi/string-input-visitor.c > index b3fdd0827d..af0a841152 100644 > --- a/qapi/string-input-visitor.c > +++ b/qapi/string-input-visitor.c > @@ -19,6 +19,7 @@ > #include "qapi/qmp/qnull.h" > #include "qemu/option.h" > #include "qemu/queue.h" > +#include "qemu/cutils.h" > #include "qemu/range.h" > > > @@ -44,7 +45,8 @@ static void free_range(void *range, void *dummy) > g_free(range); > } > > -static int parse_str(StringInputVisitor *siv, const char *name, Error **errp) > +static int parse_str_int64(StringInputVisitor *siv, const char *name, > + Error **errp) > { > char *str = (char *) siv->string; > long long start, end; > @@ -118,6 +120,75 @@ error: > return -1; > } > > +static int parse_str_uint64(StringInputVisitor *siv, const char *name, > + Error **errp) > +{ > + const char *str = (char *) siv->string; > + uint64_t start, end; > + const char *endptr; > + Range *cur; > + > + if (siv->ranges) { > + return 0; > + } > + > + if (!*str) { > + return 0; > + } > + > + do { > + if (!qemu_strtou64(str, &endptr, 0, &start)) { > + if (*endptr == '\0') { > + cur = g_malloc0(sizeof(*cur)); > + range_set_bounds(cur, start, start); > + siv->ranges = range_list_insert(siv->ranges, cur); > + cur = NULL; > + str = NULL; > + } else if (*endptr == '-') { > + str = endptr + 1; > + if (!qemu_strtou64(str, &endptr, 0, &end) && start <= end) { > + if (*endptr == '\0') { > + cur = g_malloc0(sizeof(*cur)); > + range_set_bounds(cur, start, end); > + siv->ranges = range_list_insert(siv->ranges, cur); > + cur = NULL; > + str = NULL; > + } else if (*endptr == ',') { > + str = endptr + 1; > + cur = g_malloc0(sizeof(*cur)); > + range_set_bounds(cur, start, end); > + siv->ranges = range_list_insert(siv->ranges, cur); > + cur = NULL; > + } else { > + goto error; > + } > + } else { > + goto error; > + } > + } else if (*endptr == ',') { > + str = endptr + 1; > + cur = g_malloc0(sizeof(*cur)); > + range_set_bounds(cur, start, start); > + siv->ranges = range_list_insert(siv->ranges, cur); > + cur = NULL; > + } else { > + goto error; > + } > + } else { > + goto error; > + } > + } while (str); > + > + return 0; > +error: > + g_list_foreach(siv->ranges, free_range, NULL); > + g_list_free(siv->ranges); > + siv->ranges = NULL; > + error_setg(errp, QERR_INVALID_PARAMETER_VALUE, name ? name : "null", > + "an uint64 value or range"); > + return -1; > +} > + Do we actually need unsigned ranges? I'm asking because I hate this code, and duplicating can only make it worse. [...]
On 17/10/2018 14:42, Markus Armbruster wrote: > Quick peek only for now. > > David Hildenbrand <david@redhat.com> writes: > >> Right now, we parse uint64_t values just like int64_t values, resulting >> in negative values getting accepted and certain valid large numbers only >> being representable as negative numbers. Also, reported errors indicate >> that an int64_t is expected. >> >> Parse uin64_t separately. Implementation inspired by original >> parse_str() implementation. >> >> E.g. we can now specify >> -device nvdimm,memdev=mem1,id=nv1,addr=0xFFFFFFFFC0000000 >> Instead of going via negative values >> -device nvdimm,memdev=mem1,id=nv1,addr=-0x40000000 >> >> Resulting in the same values >> >> (qemu) info memory-devices >> Memory device [nvdimm]: "nv1" >> addr: 0xffffffffc0000000 >> slot: 0 >> node: 0 >> >> Signed-off-by: David Hildenbrand <david@redhat.com> > > Related work on the QObject input visitor: > > commit 5923f85fb82df7c8c60a89458a5ae856045e5ab1 > Author: Marc-André Lureau <marcandre.lureau@redhat.com> > Date: Wed Jun 7 20:36:03 2017 +0400 > > qapi: update the qobject visitor to use QNUM_U64 > > Switch to use QNum/uint where appropriate to remove i64 limitation. > > The input visitor will cast i64 input to u64 for compatibility > reasons (existing json QMP client already use negative i64 for large > u64, and expect an implicit cast in qemu). > > Note: before the patch, uint64_t values above INT64_MAX are sent over > json QMP as negative values, e.g. UINT64_MAX is sent as -1. After the > patch, they are sent unmodified. Clearly a bug fix, but we have to > consider compatibility issues anyway. libvirt should cope fine, > because its parsing of unsigned integers accepts negative values > modulo 2^64. There's hope that other clients will, too. > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> > Reviewed-by: Markus Armbruster <armbru@redhat.com> > Message-Id: <20170607163635.17635-12-marcandre.lureau@redhat.com> > [check_native_list() tweaked for consistency with signed case] > Signed-off-by: Markus Armbruster <armbru@redhat.com> > > Note who it considers backward compatibility. Have you done that for > the string input visitor? The commit message should tell. There should be no compat issues, negative values are still accepted. At least I can't think of any :) We simply allow accepting bigger values. > >> --- >> qapi/string-input-visitor.c | 117 ++++++++++++++++++++++++++++++++---- >> 1 file changed, 106 insertions(+), 11 deletions(-) >> >> diff --git a/qapi/string-input-visitor.c b/qapi/string-input-visitor.c >> index b3fdd0827d..af0a841152 100644 >> --- a/qapi/string-input-visitor.c >> +++ b/qapi/string-input-visitor.c >> @@ -19,6 +19,7 @@ >> #include "qapi/qmp/qnull.h" >> #include "qemu/option.h" >> #include "qemu/queue.h" >> +#include "qemu/cutils.h" >> #include "qemu/range.h" >> >> >> @@ -44,7 +45,8 @@ static void free_range(void *range, void *dummy) >> g_free(range); >> } >> >> -static int parse_str(StringInputVisitor *siv, const char *name, Error **errp) >> +static int parse_str_int64(StringInputVisitor *siv, const char *name, >> + Error **errp) >> { >> char *str = (char *) siv->string; >> long long start, end; >> @@ -118,6 +120,75 @@ error: >> return -1; >> } >> >> +static int parse_str_uint64(StringInputVisitor *siv, const char *name, >> + Error **errp) >> +{ >> + const char *str = (char *) siv->string; >> + uint64_t start, end; >> + const char *endptr; >> + Range *cur; >> + >> + if (siv->ranges) { >> + return 0; >> + } >> + >> + if (!*str) { >> + return 0; >> + } >> + >> + do { >> + if (!qemu_strtou64(str, &endptr, 0, &start)) { >> + if (*endptr == '\0') { >> + cur = g_malloc0(sizeof(*cur)); >> + range_set_bounds(cur, start, start); >> + siv->ranges = range_list_insert(siv->ranges, cur); >> + cur = NULL; >> + str = NULL; >> + } else if (*endptr == '-') { >> + str = endptr + 1; >> + if (!qemu_strtou64(str, &endptr, 0, &end) && start <= end) { >> + if (*endptr == '\0') { >> + cur = g_malloc0(sizeof(*cur)); >> + range_set_bounds(cur, start, end); >> + siv->ranges = range_list_insert(siv->ranges, cur); >> + cur = NULL; >> + str = NULL; >> + } else if (*endptr == ',') { >> + str = endptr + 1; >> + cur = g_malloc0(sizeof(*cur)); >> + range_set_bounds(cur, start, end); >> + siv->ranges = range_list_insert(siv->ranges, cur); >> + cur = NULL; >> + } else { >> + goto error; >> + } >> + } else { >> + goto error; >> + } >> + } else if (*endptr == ',') { >> + str = endptr + 1; >> + cur = g_malloc0(sizeof(*cur)); >> + range_set_bounds(cur, start, start); >> + siv->ranges = range_list_insert(siv->ranges, cur); >> + cur = NULL; >> + } else { >> + goto error; >> + } >> + } else { >> + goto error; >> + } >> + } while (str); >> + >> + return 0; >> +error: >> + g_list_foreach(siv->ranges, free_range, NULL); >> + g_list_free(siv->ranges); >> + siv->ranges = NULL; >> + error_setg(errp, QERR_INVALID_PARAMETER_VALUE, name ? name : "null", >> + "an uint64 value or range"); >> + return -1; >> +} >> + > > Do we actually need unsigned ranges? I'm asking because I hate this > code, and duplicating can only make it worse. > > [...] I don't think we need unsigned ranges BUT I am concerned about backwards compatibility. I'll have to check all users to make sure no property flagged as uint64_t will actually expect ranges. Then we can drop it. (and simplify this code)
On 23/10/2018 14:10, David Hildenbrand wrote: > On 17/10/2018 14:42, Markus Armbruster wrote: >> Quick peek only for now. >> >> David Hildenbrand <david@redhat.com> writes: >> >>> Right now, we parse uint64_t values just like int64_t values, resulting >>> in negative values getting accepted and certain valid large numbers only >>> being representable as negative numbers. Also, reported errors indicate >>> that an int64_t is expected. >>> >>> Parse uin64_t separately. Implementation inspired by original >>> parse_str() implementation. >>> >>> E.g. we can now specify >>> -device nvdimm,memdev=mem1,id=nv1,addr=0xFFFFFFFFC0000000 >>> Instead of going via negative values >>> -device nvdimm,memdev=mem1,id=nv1,addr=-0x40000000 >>> >>> Resulting in the same values >>> >>> (qemu) info memory-devices >>> Memory device [nvdimm]: "nv1" >>> addr: 0xffffffffc0000000 >>> slot: 0 >>> node: 0 >>> >>> Signed-off-by: David Hildenbrand <david@redhat.com> >> >> Related work on the QObject input visitor: >> >> commit 5923f85fb82df7c8c60a89458a5ae856045e5ab1 >> Author: Marc-André Lureau <marcandre.lureau@redhat.com> >> Date: Wed Jun 7 20:36:03 2017 +0400 >> >> qapi: update the qobject visitor to use QNUM_U64 >> >> Switch to use QNum/uint where appropriate to remove i64 limitation. >> >> The input visitor will cast i64 input to u64 for compatibility >> reasons (existing json QMP client already use negative i64 for large >> u64, and expect an implicit cast in qemu). >> >> Note: before the patch, uint64_t values above INT64_MAX are sent over >> json QMP as negative values, e.g. UINT64_MAX is sent as -1. After the >> patch, they are sent unmodified. Clearly a bug fix, but we have to >> consider compatibility issues anyway. libvirt should cope fine, >> because its parsing of unsigned integers accepts negative values >> modulo 2^64. There's hope that other clients will, too. >> >> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> >> Reviewed-by: Markus Armbruster <armbru@redhat.com> >> Message-Id: <20170607163635.17635-12-marcandre.lureau@redhat.com> >> [check_native_list() tweaked for consistency with signed case] >> Signed-off-by: Markus Armbruster <armbru@redhat.com> >> >> Note who it considers backward compatibility. Have you done that for >> the string input visitor? The commit message should tell. > > There should be no compat issues, negative values are still accepted. At > least I can't think of any :) We simply allow accepting bigger values. > >> >>> --- >>> qapi/string-input-visitor.c | 117 ++++++++++++++++++++++++++++++++---- >>> 1 file changed, 106 insertions(+), 11 deletions(-) >>> >>> diff --git a/qapi/string-input-visitor.c b/qapi/string-input-visitor.c >>> index b3fdd0827d..af0a841152 100644 >>> --- a/qapi/string-input-visitor.c >>> +++ b/qapi/string-input-visitor.c >>> @@ -19,6 +19,7 @@ >>> #include "qapi/qmp/qnull.h" >>> #include "qemu/option.h" >>> #include "qemu/queue.h" >>> +#include "qemu/cutils.h" >>> #include "qemu/range.h" >>> >>> >>> @@ -44,7 +45,8 @@ static void free_range(void *range, void *dummy) >>> g_free(range); >>> } >>> >>> -static int parse_str(StringInputVisitor *siv, const char *name, Error **errp) >>> +static int parse_str_int64(StringInputVisitor *siv, const char *name, >>> + Error **errp) >>> { >>> char *str = (char *) siv->string; >>> long long start, end; >>> @@ -118,6 +120,75 @@ error: >>> return -1; >>> } >>> >>> +static int parse_str_uint64(StringInputVisitor *siv, const char *name, >>> + Error **errp) >>> +{ >>> + const char *str = (char *) siv->string; >>> + uint64_t start, end; >>> + const char *endptr; >>> + Range *cur; >>> + >>> + if (siv->ranges) { >>> + return 0; >>> + } >>> + >>> + if (!*str) { >>> + return 0; >>> + } >>> + >>> + do { >>> + if (!qemu_strtou64(str, &endptr, 0, &start)) { >>> + if (*endptr == '\0') { >>> + cur = g_malloc0(sizeof(*cur)); >>> + range_set_bounds(cur, start, start); >>> + siv->ranges = range_list_insert(siv->ranges, cur); >>> + cur = NULL; >>> + str = NULL; >>> + } else if (*endptr == '-') { >>> + str = endptr + 1; >>> + if (!qemu_strtou64(str, &endptr, 0, &end) && start <= end) { >>> + if (*endptr == '\0') { >>> + cur = g_malloc0(sizeof(*cur)); >>> + range_set_bounds(cur, start, end); >>> + siv->ranges = range_list_insert(siv->ranges, cur); >>> + cur = NULL; >>> + str = NULL; >>> + } else if (*endptr == ',') { >>> + str = endptr + 1; >>> + cur = g_malloc0(sizeof(*cur)); >>> + range_set_bounds(cur, start, end); >>> + siv->ranges = range_list_insert(siv->ranges, cur); >>> + cur = NULL; >>> + } else { >>> + goto error; >>> + } >>> + } else { >>> + goto error; >>> + } >>> + } else if (*endptr == ',') { >>> + str = endptr + 1; >>> + cur = g_malloc0(sizeof(*cur)); >>> + range_set_bounds(cur, start, start); >>> + siv->ranges = range_list_insert(siv->ranges, cur); >>> + cur = NULL; >>> + } else { >>> + goto error; >>> + } >>> + } else { >>> + goto error; >>> + } >>> + } while (str); >>> + >>> + return 0; >>> +error: >>> + g_list_foreach(siv->ranges, free_range, NULL); >>> + g_list_free(siv->ranges); >>> + siv->ranges = NULL; >>> + error_setg(errp, QERR_INVALID_PARAMETER_VALUE, name ? name : "null", >>> + "an uint64 value or range"); >>> + return -1; >>> +} >>> + >> >> Do we actually need unsigned ranges? I'm asking because I hate this >> code, and duplicating can only make it worse. >> >> [...] > > I don't think we need unsigned ranges BUT I am concerned about backwards > compatibility. I'll have to check all users to make sure no property > flagged as uint64_t will actually expect ranges. Then we can drop it. > (and simplify this code) > > ... looking at the details, I think you are right, we should not need that range code at all. I will drop it. Makes things a lot simpler :)
diff --git a/qapi/string-input-visitor.c b/qapi/string-input-visitor.c index b3fdd0827d..af0a841152 100644 --- a/qapi/string-input-visitor.c +++ b/qapi/string-input-visitor.c @@ -19,6 +19,7 @@ #include "qapi/qmp/qnull.h" #include "qemu/option.h" #include "qemu/queue.h" +#include "qemu/cutils.h" #include "qemu/range.h" @@ -44,7 +45,8 @@ static void free_range(void *range, void *dummy) g_free(range); } -static int parse_str(StringInputVisitor *siv, const char *name, Error **errp) +static int parse_str_int64(StringInputVisitor *siv, const char *name, + Error **errp) { char *str = (char *) siv->string; long long start, end; @@ -118,6 +120,75 @@ error: return -1; } +static int parse_str_uint64(StringInputVisitor *siv, const char *name, + Error **errp) +{ + const char *str = (char *) siv->string; + uint64_t start, end; + const char *endptr; + Range *cur; + + if (siv->ranges) { + return 0; + } + + if (!*str) { + return 0; + } + + do { + if (!qemu_strtou64(str, &endptr, 0, &start)) { + if (*endptr == '\0') { + cur = g_malloc0(sizeof(*cur)); + range_set_bounds(cur, start, start); + siv->ranges = range_list_insert(siv->ranges, cur); + cur = NULL; + str = NULL; + } else if (*endptr == '-') { + str = endptr + 1; + if (!qemu_strtou64(str, &endptr, 0, &end) && start <= end) { + if (*endptr == '\0') { + cur = g_malloc0(sizeof(*cur)); + range_set_bounds(cur, start, end); + siv->ranges = range_list_insert(siv->ranges, cur); + cur = NULL; + str = NULL; + } else if (*endptr == ',') { + str = endptr + 1; + cur = g_malloc0(sizeof(*cur)); + range_set_bounds(cur, start, end); + siv->ranges = range_list_insert(siv->ranges, cur); + cur = NULL; + } else { + goto error; + } + } else { + goto error; + } + } else if (*endptr == ',') { + str = endptr + 1; + cur = g_malloc0(sizeof(*cur)); + range_set_bounds(cur, start, start); + siv->ranges = range_list_insert(siv->ranges, cur); + cur = NULL; + } else { + goto error; + } + } else { + goto error; + } + } while (str); + + return 0; +error: + g_list_foreach(siv->ranges, free_range, NULL); + g_list_free(siv->ranges); + siv->ranges = NULL; + error_setg(errp, QERR_INVALID_PARAMETER_VALUE, name ? name : "null", + "an uint64 value or range"); + return -1; +} + static void start_list(Visitor *v, const char *name, GenericList **list, size_t size, Error **errp) @@ -128,7 +199,7 @@ start_list(Visitor *v, const char *name, GenericList **list, size_t size, assert(list); siv->list = list; - if (parse_str(siv, name, errp) < 0) { + if (parse_str_int64(siv, name, errp) < 0) { *list = NULL; return; } @@ -216,7 +287,7 @@ static void parse_type_int64(Visitor *v, const char *name, int64_t *obj, { StringInputVisitor *siv = to_siv(v); - if (parse_str(siv, name, errp) < 0) { + if (parse_str_int64(siv, name, errp) < 0) { return; } @@ -252,15 +323,39 @@ error: static void parse_type_uint64(Visitor *v, const char *name, uint64_t *obj, Error **errp) { - /* FIXME: parse_type_int64 mishandles values over INT64_MAX */ - int64_t i; - Error *err = NULL; - parse_type_int64(v, name, &i, &err); - if (err) { - error_propagate(errp, err); - } else { - *obj = i; + StringInputVisitor *siv = to_siv(v); + + if (parse_str_uint64(siv, name, errp) < 0) { + return; + } + + if (!siv->ranges) { + goto error; + } + + if (!siv->cur_range) { + Range *r; + + siv->cur_range = g_list_first(siv->ranges); + if (!siv->cur_range) { + goto error; + } + + r = siv->cur_range->data; + if (!r) { + goto error; + } + + siv->cur = range_lob(r); } + + *obj = siv->cur; + siv->cur++; + return; + +error: + error_setg(errp, QERR_INVALID_PARAMETER_VALUE, name ? name : "null", + "an uint64 value or range"); } static void parse_type_size(Visitor *v, const char *name, uint64_t *obj,
Right now, we parse uint64_t values just like int64_t values, resulting in negative values getting accepted and certain valid large numbers only being representable as negative numbers. Also, reported errors indicate that an int64_t is expected. Parse uin64_t separately. Implementation inspired by original parse_str() implementation. E.g. we can now specify -device nvdimm,memdev=mem1,id=nv1,addr=0xFFFFFFFFC0000000 Instead of going via negative values -device nvdimm,memdev=mem1,id=nv1,addr=-0x40000000 Resulting in the same values (qemu) info memory-devices Memory device [nvdimm]: "nv1" addr: 0xffffffffc0000000 slot: 0 node: 0 Signed-off-by: David Hildenbrand <david@redhat.com> --- qapi/string-input-visitor.c | 117 ++++++++++++++++++++++++++++++++---- 1 file changed, 106 insertions(+), 11 deletions(-)