Il 26/03/2014 11:37, hu...@cn.fujitsu.com ha scritto: > Signed-off-by: Hu Tao <hu...@cn.fujitsu.com>
Just a small comment below. > --- > qapi/string-output-visitor.c | 236 > +++++++++++++++++++++++++++++++++++-- > tests/test-string-output-visitor.c | 35 ++++++ > 2 files changed, 260 insertions(+), 11 deletions(-) > > diff --git a/qapi/string-output-visitor.c b/qapi/string-output-visitor.c > index fb1d2e8..edfc0a4 100644 > --- a/qapi/string-output-visitor.c > +++ b/qapi/string-output-visitor.c > @@ -16,32 +16,177 @@ > #include "qapi/qmp/qerror.h" > #include "qemu/host-utils.h" > #include <math.h> > +#include "qemu/range.h" > + > +enum ListMode { > + LM_NONE, /* not traversing a list of repeated options */ > + LM_STARTED, /* start_list() succeeded */ > + > + LM_IN_PROGRESS, /* next_list() has been called. > + * > + * Generating the next list link will consume the > most > + * recently parsed QemuOpt instance of the repeated > + * option. > + * > + * Parsing a value into the list link will examine > the > + * next QemuOpt instance of the repeated option, and > + * possibly enter LM_SIGNED_INTERVAL or > + * LM_UNSIGNED_INTERVAL. > + */ > + > + LM_SIGNED_INTERVAL, /* next_list() has been called. > + * > + * Generating the next list link will consume the > most > + * recently stored element from the signed interval, > + * parsed from the most recent QemuOpt instance of > the > + * repeated option. This may consume QemuOpt itself > + * and return to LM_IN_PROGRESS. > + * > + * Parsing a value into the list link will store the > + * next element of the signed interval. > + */ > + > + LM_UNSIGNED_INTERVAL,/* Same as above, only for an unsigned interval. */ > + > + LM_END > +}; > + > +typedef enum ListMode ListMode; > > struct StringOutputVisitor > { > Visitor visitor; > bool human; > - char *string; > + GString *string; > + bool head; > + ListMode list_mode; > + union { > + int64_t s; > + uint64_t u; > + } range_start, range_end; > + SignedRangeList *ranges; > }; > > static void string_output_set(StringOutputVisitor *sov, char *string) > { > - g_free(sov->string); > - sov->string = string; > + if (sov->string) { > + g_string_free(sov->string, true); > + } > + sov->string = g_string_new(string); > + g_free(string); > +} > + > +static void string_output_append(StringOutputVisitor *sov, int64_t a) > +{ > + range_list_add(sov->ranges, a, 1); > +} > + > +static void string_output_append_range(StringOutputVisitor *sov, > + int64_t s, int64_t e) > +{ > + range_list_add(sov->ranges, s, e); > } > > static void print_type_int(Visitor *v, int64_t *obj, const char *name, > Error **errp) > { > StringOutputVisitor *sov = DO_UPCAST(StringOutputVisitor, visitor, v); > - char *out; > + SignedRange *r; > > - if (sov->human) { > - out = g_strdup_printf("%lld (%#llx)", (long long) *obj, (long long) > *obj); > - } else { > - out = g_strdup_printf("%lld", (long long) *obj); > + if (!sov->ranges) { > + sov->ranges = g_malloc0(sizeof(*sov->ranges)); > + QTAILQ_INIT(sov->ranges); > + } > + > + switch (sov->list_mode) { > + case LM_NONE: > + string_output_append(sov, *obj); > + goto format_string; > + break; > + No need for the goto if... > + case LM_STARTED: > + sov->range_start.s = *obj; > + sov->range_end.s = *obj; > + sov->list_mode = LM_IN_PROGRESS; > + break; ... you have "return;" here > + case LM_IN_PROGRESS: > + if (sov->range_end.s + 1 == *obj) { > + sov->range_end.s++; > + break; (unnecessary break) > + } else { > + if (sov->range_start.s == sov->range_end.s) { > + string_output_append(sov, sov->range_end.s); > + } else { > + assert(sov->range_start.s < sov->range_end.s); > + string_output_append_range(sov, sov->range_start.s, > + sov->range_end.s - > + sov->range_start.s + 1); > + } > + > + sov->range_start.s = *obj; > + sov->range_end.s = *obj; > + } > + break; ... and "return;" here... > + case LM_END: > + if (sov->range_end.s + 1 == *obj) { > + sov->range_end.s++; > + assert(sov->range_start.s < sov->range_end.s); > + string_output_append_range(sov, sov->range_start.s, > + sov->range_end.s - > + sov->range_start.s + 1); > + } else { > + if (sov->range_start.s == sov->range_end.s) { > + string_output_append(sov, sov->range_end.s); > + } else { > + assert(sov->range_start.s < sov->range_end.s); > + > + string_output_append_range(sov, sov->range_start.s, > + sov->range_end.s - > + sov->range_start.s + 1); > + } > + string_output_append(sov, *obj); > + } ... and "break;" here" and move the formatting code outside the "switch". > +format_string: > + QTAILQ_FOREACH(r, sov->ranges, entry) { > + if (r->length > 1) { > + g_string_append_printf(sov->string, "%" PRId64 "-%" PRId64, > + r->start, > + s_range_end(r->start, r->length)); > + } else { > + g_string_append_printf(sov->string, "%" PRId64, > + r->start); > + } > + if (r->entry.tqe_next) { > + g_string_append(sov->string, ","); > + } > + } > + > + if (sov->human) { > + g_string_append(sov->string, " ("); > + QTAILQ_FOREACH(r, sov->ranges, entry) { > + if (r->length > 1) { > + g_string_append_printf(sov->string, "%" PRIx64 "-%" > PRIx64, > + r->start, > + s_range_end(r->start, r->length)); > + } else { > + g_string_append_printf(sov->string, "%" PRIx64, > + r->start); > + } > + if (r->entry.tqe_next) { > + g_string_append(sov->string, ","); > + } > + } > + g_string_append(sov->string, ")"); > + } > + > + break; > + > + default: > + abort(); > } > - string_output_set(sov, out); > } > > static void print_type_size(Visitor *v, uint64_t *obj, const char *name, > @@ -103,9 +248,61 @@ static void print_type_number(Visitor *v, double *obj, > const char *name, > string_output_set(sov, g_strdup_printf("%f", *obj)); > } > > +static void > +start_list(Visitor *v, const char *name, Error **errp) > +{ > + StringOutputVisitor *sov = DO_UPCAST(StringOutputVisitor, visitor, v); > + > + /* we can't traverse a list in a list */ > + assert(sov->list_mode == LM_NONE); > + sov->list_mode = LM_STARTED; > + sov->head = true; > +} > + > +static GenericList * > +next_list(Visitor *v, GenericList **list, Error **errp) > +{ > + StringOutputVisitor *sov = DO_UPCAST(StringOutputVisitor, visitor, v); > + GenericList *ret = NULL; > + if (*list) { > + if (sov->head) { > + ret = *list; > + } else { > + ret = (*list)->next; > + } > + > + if (sov->head) { > + if (ret && ret->next == NULL) { > + sov->list_mode = LM_NONE; > + } > + sov->head = false; > + } else { > + if (ret && ret->next == NULL) { > + sov->list_mode = LM_END; > + } > + } > + } > + > + return ret; > +} > + > +static void > +end_list(Visitor *v, Error **errp) > +{ > + StringOutputVisitor *sov = DO_UPCAST(StringOutputVisitor, visitor, v); > + > + assert(sov->list_mode == LM_STARTED || > + sov->list_mode == LM_END || > + sov->list_mode == LM_NONE || > + sov->list_mode == LM_IN_PROGRESS); > + sov->list_mode = LM_NONE; > + sov->head = true; > + > +} > + > char *string_output_get_string(StringOutputVisitor *sov) > { > - char *string = sov->string; > + char *string = g_string_free(sov->string, false); > sov->string = NULL; > return string; > } > @@ -117,7 +314,20 @@ Visitor *string_output_get_visitor(StringOutputVisitor > *sov) > > void string_output_visitor_cleanup(StringOutputVisitor *sov) > { > - g_free(sov->string); > + SignedRange *r, *next; > + > + if (sov->string) { > + g_string_free(sov->string, true); > + } > + > + if (sov->ranges) { > + QTAILQ_FOREACH_SAFE(r, sov->ranges, entry, next) { > + QTAILQ_REMOVE(sov->ranges, r, entry); > + s_range_free(r); > + } > + g_free(sov->ranges); > + } > + > g_free(sov); > } > > @@ -127,6 +337,7 @@ StringOutputVisitor *string_output_visitor_new(bool human) > > v = g_malloc0(sizeof(*v)); > > + v->string = g_string_new(NULL); > v->human = human; > v->visitor.type_enum = output_type_enum; > v->visitor.type_int = print_type_int; > @@ -134,6 +345,9 @@ StringOutputVisitor *string_output_visitor_new(bool human) > v->visitor.type_bool = print_type_bool; > v->visitor.type_str = print_type_str; > v->visitor.type_number = print_type_number; > + v->visitor.start_list = start_list; > + v->visitor.next_list = next_list; > + v->visitor.end_list = end_list; > > return v; > } > diff --git a/tests/test-string-output-visitor.c > b/tests/test-string-output-visitor.c > index 22363d1..a460377 100644 > --- a/tests/test-string-output-visitor.c > +++ b/tests/test-string-output-visitor.c > @@ -57,6 +57,39 @@ static void test_visitor_out_int(TestOutputVisitorData > *data, > g_free(str); > } > > +static void test_visitor_out_intList(TestOutputVisitorData *data, > + const void *unused) > +{ > + int64_t value[] = {-10, -7, -2, -1, 0, 1, 9, 10, 16, 15, 14, > + 3, 4, 5, 6, 11, 12, 13, 21, 22, INT64_MAX - 1, INT64_MAX}; > + intList *list = NULL, **tmp = &list; > + int i; > + Error *errp = NULL; > + char *str; > + > + for (i = 0; i < sizeof(value) / sizeof(value[0]); i++) { > + *tmp = g_malloc0(sizeof(**tmp)); > + (*tmp)->value = value[i]; > + tmp = &(*tmp)->next; > + } > + > + visit_type_intList(data->ov, &list, NULL, &errp); > + g_assert(error_is_set(&errp) == 0); > + > + str = string_output_get_string(data->sov); > + g_assert(str != NULL); > + g_assert_cmpstr(str, ==, > + > "-10,-7,-2-1,3-6,9-16,21-22,9223372036854775806-9223372036854775807"); > + g_free(str); > +#if 0 > + while (list) { > + tmp2 = list->next; > + g_free(list); > + list = tmp2; > + } > +#endif > +} > + > static void test_visitor_out_bool(TestOutputVisitorData *data, > const void *unused) > { > @@ -182,6 +215,8 @@ int main(int argc, char **argv) > &out_visitor_data, test_visitor_out_enum); > output_visitor_test_add("/string-visitor/output/enum-errors", > &out_visitor_data, test_visitor_out_enum_errors); > + output_visitor_test_add("/string-visitor/output/intList", > + &out_visitor_data, test_visitor_out_intList); > > g_test_run(); > >