Hi,
On Mon, Mar 10, 2014 at 03:31:39PM +0800, Chunyan Liu wrote:
> Improve opt_get and opt_set group of functions. For opt_get, check and handle
> NULL input; for opt_set, when set to an existing option, rewrite the option
> with new value.
>
> Signed-off-by: Dong Xu Wang <wdon...@linux.vnet.ibm.com>
> Signed-off-by: Chunyan Liu <cy...@suse.com>
> ---
> include/qemu/option_int.h | 4 +--
> util/qemu-option.c | 81
> +++++++++++++++++++++++++++++++++++++++--------
> 2 files changed, 69 insertions(+), 16 deletions(-)
>
> diff --git a/include/qemu/option_int.h b/include/qemu/option_int.h
> index 8212fa4..db9ed91 100644
> --- a/include/qemu/option_int.h
> +++ b/include/qemu/option_int.h
> @@ -30,8 +30,8 @@
> #include "qemu/error-report.h"
>
> struct QemuOpt {
> - const char *name;
> - const char *str;
> + char *name;
> + char *str;
>
> const QemuOptDesc *desc;
> union {
> diff --git a/util/qemu-option.c b/util/qemu-option.c
> index 65d1c22..c7639e8 100644
> --- a/util/qemu-option.c
> +++ b/util/qemu-option.c
> @@ -558,8 +558,13 @@ static QemuOpt *qemu_opt_find(QemuOpts *opts, const char
> *name)
>
> const char *qemu_opt_get(QemuOpts *opts, const char *name)
> {
> - QemuOpt *opt = qemu_opt_find(opts, name);
> + QemuOpt *opt;
>
> + if (opts == NULL) {
> + return NULL;
> + }
I understand you want to prevent a segfault in qemu_opt_find(), and I'm fine
with that,
but these checks make me wonder why you decided to change it, aren't you hiding
some
broken caller?
Btw, you could change qemu_opt_find() and check if opts != NULL there and not
introduce
all these opts == NULL qemu_opt_find() pairs.
--
Dorileo
> +
> + opt = qemu_opt_find(opts, name);
> if (!opt) {
> const QemuOptDesc *desc = find_desc_by_name(opts->list->desc, name);
> if (desc && desc->def_value_str) {
> @@ -583,7 +588,13 @@ bool qemu_opt_has_help_opt(QemuOpts *opts)
>
> bool qemu_opt_get_bool(QemuOpts *opts, const char *name, bool defval)
> {
> - QemuOpt *opt = qemu_opt_find(opts, name);
> + QemuOpt *opt;
> +
> + if (opts == NULL) {
> + return defval;
> + }
> +
> + opt = qemu_opt_find(opts, name);
>
> if (opt == NULL) {
> const QemuOptDesc *desc = find_desc_by_name(opts->list->desc, name);
> @@ -598,7 +609,13 @@ bool qemu_opt_get_bool(QemuOpts *opts, const char *name,
> bool defval)
>
> uint64_t qemu_opt_get_number(QemuOpts *opts, const char *name, uint64_t
> defval)
> {
> - QemuOpt *opt = qemu_opt_find(opts, name);
> + QemuOpt *opt;
> +
> + if (opts == NULL) {
> + return defval;
> + }
> +
> + opt = qemu_opt_find(opts, name);
>
> if (opt == NULL) {
> const QemuOptDesc *desc = find_desc_by_name(opts->list->desc, name);
> @@ -614,8 +631,13 @@ uint64_t qemu_opt_get_number(QemuOpts *opts, const char
> *name, uint64_t defval)
>
> uint64_t qemu_opt_get_size(QemuOpts *opts, const char *name, uint64_t defval)
> {
> - QemuOpt *opt = qemu_opt_find(opts, name);
> + QemuOpt *opt;
>
> + if (opts == NULL) {
> + return defval;
> + }
> +
> + opt = qemu_opt_find(opts, name);
> if (opt == NULL) {
> const QemuOptDesc *desc = find_desc_by_name(opts->list->desc, name);
> if (desc && desc->def_value_str) {
> @@ -652,6 +674,10 @@ static void qemu_opt_parse(QemuOpt *opt, Error **errp)
>
> static void qemu_opt_del(QemuOpt *opt)
> {
> + if (opt == NULL) {
> + return;
> + }
> +
> QTAILQ_REMOVE(&opt->opts->head, opt, next);
> g_free((/* !const */ char*)opt->name);
> g_free((/* !const */ char*)opt->str);
> @@ -704,6 +730,13 @@ static void opt_set(QemuOpts *opts, const char *name,
> const char *value,
> return;
> }
>
> + opt = qemu_opt_find(opts, name);
> + if (opt) {
> + g_free((char *)opt->str);
> + opt->str = g_strdup(value);
> + return;
> + }
> +
> opt = g_malloc0(sizeof(*opt));
> opt->name = g_strdup(name);
> opt->opts = opts;
> @@ -744,16 +777,24 @@ void qemu_opt_set_err(QemuOpts *opts, const char *name,
> const char *value,
> int qemu_opt_set_bool(QemuOpts *opts, const char *name, bool val)
> {
> QemuOpt *opt;
> - const QemuOptDesc *desc = opts->list->desc;
> + const QemuOptDesc *desc;
>
> - opt = g_malloc0(sizeof(*opt));
> - opt->desc = find_desc_by_name(desc, name);
> - if (!opt->desc && !opts_accepts_any(opts)) {
> + desc = find_desc_by_name(opts->list->desc, name);
> + if (!desc && !opts_accepts_any(opts)) {
> qerror_report(QERR_INVALID_PARAMETER, name);
> - g_free(opt);
> return -1;
> }
>
> + opt = qemu_opt_find(opts, name);
> + if (opt) {
> + g_free((char *)opt->str);
> + opt->value.boolean = val;
> + opt->str = g_strdup(val ? "on" : "off");
> + return 0;
> + }
> +
> + opt = g_malloc0(sizeof(*opt));
> + opt->desc = desc;
> opt->name = g_strdup(name);
> opt->opts = opts;
> opt->value.boolean = !!val;
> @@ -766,16 +807,24 @@ int qemu_opt_set_bool(QemuOpts *opts, const char *name,
> bool val)
> int qemu_opt_set_number(QemuOpts *opts, const char *name, int64_t val)
> {
> QemuOpt *opt;
> - const QemuOptDesc *desc = opts->list->desc;
> + const QemuOptDesc *desc;
>
> - opt = g_malloc0(sizeof(*opt));
> - opt->desc = find_desc_by_name(desc, name);
> - if (!opt->desc && !opts_accepts_any(opts)) {
> + desc = find_desc_by_name(opts->list->desc, name);
> + if (!desc && !opts_accepts_any(opts)) {
> qerror_report(QERR_INVALID_PARAMETER, name);
> - g_free(opt);
> return -1;
> }
>
> + opt = qemu_opt_find(opts, name);
> + if (opt) {
> + g_free((char *)opt->str);
> + opt->value.uint = val;
> + opt->str = g_strdup_printf("%" PRId64, val);
> + return 0;
> + }
> +
> + opt = g_malloc0(sizeof(*opt));
> + opt->desc = desc;
> opt->name = g_strdup(name);
> opt->opts = opts;
> opt->value.uint = val;
> @@ -910,6 +959,10 @@ void qemu_opts_del(QemuOpts *opts)
> {
> QemuOpt *opt;
>
> + if (opts == NULL) {
> + return;
> + }
> +
> for (;;) {
> opt = QTAILQ_FIRST(&opts->head);
> if (opt == NULL)
> --
> 1.7.12.4
>
>
