| Message ID | 20231004120019.93101-15-philmd@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | (few more) Steps towards enabling -Wshadow | expand |
Philippe Mathieu-Daudé <philmd@linaro.org> writes: > Fix: > > softmmu/vl.c:1069:44: error: declaration shadows a variable in the global scope [-Werror,-Wshadow] > static void parse_display_qapi(const char *optarg) > ^ > softmmu/vl.c:1224:39: error: declaration shadows a variable in the global scope [-Werror,-Wshadow] > static void monitor_parse(const char *optarg, const char *mode, bool pretty) > ^ > softmmu/vl.c:1634:17: error: declaration shadows a variable in the global scope [-Werror,-Wshadow] > const char *optarg = qdict_get_try_str(qdict, "type"); > ^ > softmmu/vl.c:1784:45: error: declaration shadows a variable in the global scope [-Werror,-Wshadow] > static void object_option_parse(const char *optarg) > ^ > /Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/usr/include/getopt.h:77:14: note: previous declaration is here > extern char *optarg; /* getopt(3) external variables */ > ^ > > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> How much we care about the shadowing is unclear, but that doesn't matter if the patches make sense even if we pretend global @optarg doesn't exist. Let's check that. > --- > softmmu/vl.c | 26 +++++++++++++------------- > 1 file changed, 13 insertions(+), 13 deletions(-) > > diff --git a/softmmu/vl.c b/softmmu/vl.c > index 98e071e63b..ae1ff9887d 100644 > --- a/softmmu/vl.c > +++ b/softmmu/vl.c > @@ -1066,12 +1066,12 @@ static void select_vgahw(const MachineClass *machine_class, const char *p) > } > } > > -static void parse_display_qapi(const char *optarg) > +static void parse_display_qapi(const char *optstr) > { > DisplayOptions *opts; > Visitor *v; > > - v = qobject_input_visitor_new_str(optarg, "type", &error_fatal); > + v = qobject_input_visitor_new_str(optstr, "type", &error_fatal); > > visit_type_DisplayOptions(v, NULL, &opts, &error_fatal); > QAPI_CLONE_MEMBERS(DisplayOptions, &dpy, opts); The actual argument is a string that is either JSON or KEY=VALUE,... The fact that it's always an option argument now (actually the value of global @optarg) is irrelevant here. parse_display_qapi() passes its parameter to qobject_input_visitor_new_str() parameter @str, which passes it to qobject_from_json() parameter @string if JSON, or else to keyval_parse() parameter @params. I'd rename @optarg to @str here, like you do in the next hunk, to not suggest a connection to CLI. Not a demand. > @@ -1221,21 +1221,21 @@ static int mon_init_func(void *opaque, QemuOpts *opts, Error **errp) > return monitor_init_opts(opts, errp); > } > > -static void monitor_parse(const char *optarg, const char *mode, bool pretty) > +static void monitor_parse(const char *str, const char *mode, bool pretty) > { > static int monitor_device_index = 0; > QemuOpts *opts; > const char *p; > char label[32]; > > - if (strstart(optarg, "chardev:", &p)) { > + if (strstart(str, "chardev:", &p)) { > snprintf(label, sizeof(label), "%s", p); > } else { > snprintf(label, sizeof(label), "compat_monitor%d", > monitor_device_index); > - opts = qemu_chr_parse_compat(label, optarg, true); > + opts = qemu_chr_parse_compat(label, str, true); > if (!opts) { > - error_report("parse error: %s", optarg); > + error_report("parse error: %s", str); > exit(1); > } > } The actual argument is either @optarg or a string literal, but that's again irrelevant here. > @@ -1631,13 +1631,13 @@ static const QEMUOption *lookup_opt(int argc, char **argv, > > static MachineClass *select_machine(QDict *qdict, Error **errp) > { > - const char *optarg = qdict_get_try_str(qdict, "type"); > + const char *machine_type = qdict_get_try_str(qdict, "type"); > GSList *machines = object_class_get_list(TYPE_MACHINE, false); > MachineClass *machine_class; > Error *local_err = NULL; > > - if (optarg) { > - machine_class = find_machine(optarg, machines); > + if (machine_type) { > + machine_class = find_machine(machine_type, machines); > qdict_del(qdict, "type"); > if (!machine_class) { > error_setg(&local_err, "unsupported machine type"); Obvious improvement. > @@ -1781,20 +1781,20 @@ static void object_option_add_visitor(Visitor *v) > QTAILQ_INSERT_TAIL(&object_opts, opt, next); > } > > -static void object_option_parse(const char *optarg) > +static void object_option_parse(const char *optstr) > { > QemuOpts *opts; > const char *type; > Visitor *v; > > - if (optarg[0] == '{') { > - QObject *obj = qobject_from_json(optarg, &error_fatal); > + if (optstr[0] == '{') { > + QObject *obj = qobject_from_json(optstr, &error_fatal); > > v = qobject_input_visitor_new(obj); > qobject_unref(obj); > } else { > opts = qemu_opts_parse_noisily(qemu_find_opts("object"), > - optarg, true); > + optstr, true); > if (!opts) { > exit(1); > } Same argument as for parse_display_qapi(), and same suggestion. If this goes though my tree, I can implement my two suggestions, if you agree. Reviewed-by: Markus Armbruster <armbru@redhat.com>
On 5/10/23 10:59, Markus Armbruster wrote: > Philippe Mathieu-Daudé <philmd@linaro.org> writes: > >> Fix: >> >> softmmu/vl.c:1069:44: error: declaration shadows a variable in the global scope [-Werror,-Wshadow] >> static void parse_display_qapi(const char *optarg) >> ^ >> softmmu/vl.c:1224:39: error: declaration shadows a variable in the global scope [-Werror,-Wshadow] >> static void monitor_parse(const char *optarg, const char *mode, bool pretty) >> ^ >> softmmu/vl.c:1634:17: error: declaration shadows a variable in the global scope [-Werror,-Wshadow] >> const char *optarg = qdict_get_try_str(qdict, "type"); >> ^ >> softmmu/vl.c:1784:45: error: declaration shadows a variable in the global scope [-Werror,-Wshadow] >> static void object_option_parse(const char *optarg) >> ^ >> /Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/usr/include/getopt.h:77:14: note: previous declaration is here >> extern char *optarg; /* getopt(3) external variables */ >> ^ >> >> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> > > How much we care about the shadowing is unclear, but that doesn't matter > if the patches make sense even if we pretend global @optarg doesn't > exist. Let's check that. > >> --- >> softmmu/vl.c | 26 +++++++++++++------------- >> 1 file changed, 13 insertions(+), 13 deletions(-) >> >> diff --git a/softmmu/vl.c b/softmmu/vl.c >> index 98e071e63b..ae1ff9887d 100644 >> --- a/softmmu/vl.c >> +++ b/softmmu/vl.c >> @@ -1066,12 +1066,12 @@ static void select_vgahw(const MachineClass *machine_class, const char *p) >> } >> } >> >> -static void parse_display_qapi(const char *optarg) >> +static void parse_display_qapi(const char *optstr) >> { >> DisplayOptions *opts; >> Visitor *v; >> >> - v = qobject_input_visitor_new_str(optarg, "type", &error_fatal); >> + v = qobject_input_visitor_new_str(optstr, "type", &error_fatal); >> >> visit_type_DisplayOptions(v, NULL, &opts, &error_fatal); >> QAPI_CLONE_MEMBERS(DisplayOptions, &dpy, opts); > > The actual argument is a string that is either JSON or KEY=VALUE,... > The fact that it's always an option argument now (actually the value of > global @optarg) is irrelevant here. > > parse_display_qapi() passes its parameter to > qobject_input_visitor_new_str() parameter @str, which passes it to > qobject_from_json() parameter @string if JSON, or else to keyval_parse() > parameter @params. > > I'd rename @optarg to @str here, like you do in the next hunk, to not > suggest a connection to CLI. Not a demand. OK. >> -static void object_option_parse(const char *optarg) >> +static void object_option_parse(const char *optstr) >> { >> QemuOpts *opts; >> const char *type; >> Visitor *v; >> >> - if (optarg[0] == '{') { >> - QObject *obj = qobject_from_json(optarg, &error_fatal); >> + if (optstr[0] == '{') { >> + QObject *obj = qobject_from_json(optstr, &error_fatal); >> >> v = qobject_input_visitor_new(obj); >> qobject_unref(obj); >> } else { >> opts = qemu_opts_parse_noisily(qemu_find_opts("object"), >> - optarg, true); >> + optstr, true); >> if (!opts) { >> exit(1); >> } > > Same argument as for parse_display_qapi(), and same suggestion. > > If this goes though my tree, I can implement my two suggestions, if you > agree. Sure, thank you! > Reviewed-by: Markus Armbruster <armbru@redhat.com> >
diff --git a/softmmu/vl.c b/softmmu/vl.c index 98e071e63b..ae1ff9887d 100644 --- a/softmmu/vl.c +++ b/softmmu/vl.c @@ -1066,12 +1066,12 @@ static void select_vgahw(const MachineClass *machine_class, const char *p) } } -static void parse_display_qapi(const char *optarg) +static void parse_display_qapi(const char *optstr) { DisplayOptions *opts; Visitor *v; - v = qobject_input_visitor_new_str(optarg, "type", &error_fatal); + v = qobject_input_visitor_new_str(optstr, "type", &error_fatal); visit_type_DisplayOptions(v, NULL, &opts, &error_fatal); QAPI_CLONE_MEMBERS(DisplayOptions, &dpy, opts); @@ -1221,21 +1221,21 @@ static int mon_init_func(void *opaque, QemuOpts *opts, Error **errp) return monitor_init_opts(opts, errp); } -static void monitor_parse(const char *optarg, const char *mode, bool pretty) +static void monitor_parse(const char *str, const char *mode, bool pretty) { static int monitor_device_index = 0; QemuOpts *opts; const char *p; char label[32]; - if (strstart(optarg, "chardev:", &p)) { + if (strstart(str, "chardev:", &p)) { snprintf(label, sizeof(label), "%s", p); } else { snprintf(label, sizeof(label), "compat_monitor%d", monitor_device_index); - opts = qemu_chr_parse_compat(label, optarg, true); + opts = qemu_chr_parse_compat(label, str, true); if (!opts) { - error_report("parse error: %s", optarg); + error_report("parse error: %s", str); exit(1); } } @@ -1631,13 +1631,13 @@ static const QEMUOption *lookup_opt(int argc, char **argv, static MachineClass *select_machine(QDict *qdict, Error **errp) { - const char *optarg = qdict_get_try_str(qdict, "type"); + const char *machine_type = qdict_get_try_str(qdict, "type"); GSList *machines = object_class_get_list(TYPE_MACHINE, false); MachineClass *machine_class; Error *local_err = NULL; - if (optarg) { - machine_class = find_machine(optarg, machines); + if (machine_type) { + machine_class = find_machine(machine_type, machines); qdict_del(qdict, "type"); if (!machine_class) { error_setg(&local_err, "unsupported machine type"); @@ -1781,20 +1781,20 @@ static void object_option_add_visitor(Visitor *v) QTAILQ_INSERT_TAIL(&object_opts, opt, next); } -static void object_option_parse(const char *optarg) +static void object_option_parse(const char *optstr) { QemuOpts *opts; const char *type; Visitor *v; - if (optarg[0] == '{') { - QObject *obj = qobject_from_json(optarg, &error_fatal); + if (optstr[0] == '{') { + QObject *obj = qobject_from_json(optstr, &error_fatal); v = qobject_input_visitor_new(obj); qobject_unref(obj); } else { opts = qemu_opts_parse_noisily(qemu_find_opts("object"), - optarg, true); + optstr, true); if (!opts) { exit(1); }
Fix: softmmu/vl.c:1069:44: error: declaration shadows a variable in the global scope [-Werror,-Wshadow] static void parse_display_qapi(const char *optarg) ^ softmmu/vl.c:1224:39: error: declaration shadows a variable in the global scope [-Werror,-Wshadow] static void monitor_parse(const char *optarg, const char *mode, bool pretty) ^ softmmu/vl.c:1634:17: error: declaration shadows a variable in the global scope [-Werror,-Wshadow] const char *optarg = qdict_get_try_str(qdict, "type"); ^ softmmu/vl.c:1784:45: error: declaration shadows a variable in the global scope [-Werror,-Wshadow] static void object_option_parse(const char *optarg) ^ /Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/usr/include/getopt.h:77:14: note: previous declaration is here extern char *optarg; /* getopt(3) external variables */ ^ Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> --- softmmu/vl.c | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-)