[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v20 03/26] improve some functions in qemu-option
From: |
Eric Blake |
Subject: |
Re: [Qemu-devel] [PATCH v20 03/26] improve some functions in qemu-option.c |
Date: |
Wed, 12 Feb 2014 16:22:45 -0700 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Thunderbird/24.3.0 |
On 02/11/2014 11:33 PM, 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
s/NUlL/NULL/
> with new value.
>
> Signed-off-by: Dong Xu Wang <address@hidden>
> Signed-off-by: Chunyan Liu <address@hidden>
> ---
> util/qemu-option.c | 84 +++++++++++++++++++++++++++++++++++++++++++--------
> 1 files changed, 70 insertions(+), 14 deletions(-)
>
> diff --git a/util/qemu-option.c b/util/qemu-option.c
> index fd84f95..ea6793a 100644
> --- a/util/qemu-option.c
> +++ b/util/qemu-option.c
> @@ -499,6 +499,9 @@ static QemuOpt *qemu_opt_find(QemuOpts *opts, const char
> *name)
> {
> QemuOpt *opt;
>
> + if (!opts)
> + return NULL;
> +
Why not just 'assert(opts);'? In other words, what caller is planning
on passing a NULL opts, since I couldn't find any existing caller that
did that? Please update the commit message with justification of how
this can simplify a caller in a later patch, if that is your plan; but
without that justification, I'm going solely off the diffstat and the
fact that no existing caller passes NULL, and concluding that this
change is bloat.
> + opt = qemu_opt_find(opts, name);
> if (!opt) {
> +
> + opt = qemu_opt_find(opts, name);
> +
> if (opt == NULL) {
Inconsistent styles here; might be worth making them all look alike
while touching them, if we keep this patch.
> @@ -664,6 +693,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);
Ugg. Why did the struct declare things as const char *str if we are
going to be strdup'ing into it? I worry about attempting to free
non-malloc'd memory any time I see a cast to lose a const. Is this just
a case of someone incorrectly trying to be const-correct, and now you
have to work around it? If you drop the 'const' from option_int.h, do
things still compile?
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
- [Qemu-devel] [PATCH v20 00/26] replace QEMUOptionParameter with QemuOpts, Chunyan Liu, 2014/02/12
- [Qemu-devel] [PATCH v20 02/26] qapi: output def_value_str when query command line options, Chunyan Liu, 2014/02/12
- [Qemu-devel] [PATCH v20 04/26] add some QemuOpts functions for replace work, Chunyan Liu, 2014/02/12
- [Qemu-devel] [PATCH v20 03/26] improve some functions in qemu-option.c, Chunyan Liu, 2014/02/12
- Re: [Qemu-devel] [PATCH v20 03/26] improve some functions in qemu-option.c,
Eric Blake <=
- [Qemu-devel] [PATCH v20 05/26] remove assertion of qemu_opt_get functions, Chunyan Liu, 2014/02/12
- [Qemu-devel] [PATCH v20 01/26] add def_value_str to QemuOptDesc, Chunyan Liu, 2014/02/12
- [Qemu-devel] [PATCH v20 06/26] change block layer to support both QemuOpts and QEMUOptionParameter, Chunyan Liu, 2014/02/12
- [Qemu-devel] [PATCH v20 08/26] gluster.c: replace QEMUOptionParameter with QemuOpts, Chunyan Liu, 2014/02/12