2014-02-13 7:22 GMT+08:00 Eric Blake <ebl...@redhat.com>:
> 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 <wdon...@linux.vnet.ibm.com>
> > Signed-off-by: Chunyan Liu <cy...@suse.com>
> > ---
> > 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.
>
>
In existing code, qemu_opt_get* and qemu_opt_unset are calling it, both
directly call qemu_opt_find without checking opts==NULL case. For these
patch
series, after adding opts check in qemu_opt_get, I can remove that from
qemu_opt_find, no problem. I just thought it would be safer to add a check.
> > + 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?
>
> I followed existing code (incorrectly using). Will remove "const" (still
compile).
Thanks.
> --
> Eric Blake eblake redhat com +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
>
>
