[2/3] chardev: fix QemuOpts lifecycle

qemu_chr_new_from_opts handles QemuOpts release now, so callers don't
have to worry.  It will either be saved in CharDriverState, then
released in qemu_chr_delete, or in the error case released instantly.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 qemu-char.c |   15 ++++++++++-----
 qemu-char.h |    1 +
 2 files changed, 11 insertions(+), 5 deletions(-)

On Wed, 17 Oct 2012 12:09:54 +0200
Gerd Hoffmann <kraxel@redhat.com> wrote:

> qemu_chr_new_from_opts handles QemuOpts release now, so callers don't
> have to worry.  It will either be saved in CharDriverState, then
> released in qemu_chr_delete, or in the error case released instantly.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  qemu-char.c |   15 ++++++++++-----
>  qemu-char.h |    1 +
>  2 files changed, 11 insertions(+), 5 deletions(-)
> 
> diff --git a/qemu-char.c b/qemu-char.c
> index e2e1da8..be4ec61 100644
> --- a/qemu-char.c
> +++ b/qemu-char.c
> @@ -2763,13 +2763,13 @@ CharDriverState *qemu_chr_new_from_opts(QemuOpts *opts,
>  
>      if (qemu_opts_id(opts) == NULL) {
>          error_setg(errp, "chardev: no id specified\n");
> -        return NULL;
> +        goto err;
>      }
>  
>      if (qemu_opt_get(opts, "backend") == NULL) {
>          error_setg(errp, "chardev: \"%s\" missing backend\n",
>                     qemu_opts_id(opts));
> -        return NULL;
> +        goto err;
>      }
>      for (i = 0; i < ARRAY_SIZE(backend_table); i++) {
>          if (strcmp(backend_table[i].name, qemu_opt_get(opts, "backend")) == 0)
> @@ -2778,14 +2778,14 @@ CharDriverState *qemu_chr_new_from_opts(QemuOpts *opts,
>      if (i == ARRAY_SIZE(backend_table)) {
>          error_setg(errp, "chardev: backend \"%s\" not found\n",
>                     qemu_opt_get(opts, "backend"));
> -        return NULL;
> +        goto err;
>      }
>  
>      chr = backend_table[i].open(opts);
>      if (!chr) {
>          error_setg(errp, "chardev: opening backend \"%s\" failed\n",
>                     qemu_opt_get(opts, "backend"));
> -        return NULL;
> +        goto err;
>      }
>  
>      if (!chr->filename)
> @@ -2806,7 +2806,12 @@ CharDriverState *qemu_chr_new_from_opts(QemuOpts *opts,
>          chr->avail_connections = 1;
>      }
>      chr->label = g_strdup(qemu_opts_id(opts));
> +    chr->opts = opts;
>      return chr;
> +
> +err:
> +    qemu_opts_del(opts);
> +    return NULL;
>  }
>  
>  CharDriverState *qemu_chr_new(const char *label, const char *filename, void (*init)(struct CharDriverState *s))
> @@ -2832,7 +2837,6 @@ CharDriverState *qemu_chr_new(const char *label, const char *filename, void (*in
>      if (chr && qemu_opt_get_bool(opts, "mux", 0)) {
>          monitor_init(chr, MONITOR_USE_READLINE);
>      }
> -    qemu_opts_del(opts);
>      return chr;
>  }
>  
> @@ -2864,6 +2868,7 @@ void qemu_chr_delete(CharDriverState *chr)
>          chr->chr_close(chr);
>      g_free(chr->filename);
>      g_free(chr->label);
> +    qemu_opts_del(chr->opts);
>      g_free(chr);
>  }

Didn't consider this in my first review, but is chardev_init_func() fine
with this? Basically, if we chardev_remove a device created by it (is this
possible?) then the opts will be freed but it will remain inserted in its
QemuOptsList.

>  
> diff --git a/qemu-char.h b/qemu-char.h
> index adae13e..99bc132 100644
> --- a/qemu-char.h
> +++ b/qemu-char.h
> @@ -74,6 +74,7 @@ struct CharDriverState {
>      char *filename;
>      int opened;
>      int avail_connections;
> +    QemuOpts *opts;
>      QTAILQ_ENTRY(CharDriverState) next;
>  };
>

Hi,

>> @@ -2864,6 +2868,7 @@ void qemu_chr_delete(CharDriverState *chr)
>>          chr->chr_close(chr);
>>      g_free(chr->filename);
>>      g_free(chr->label);
>> +    qemu_opts_del(chr->opts);
>>      g_free(chr);
>>  }
> 
> Didn't consider this in my first review, but is chardev_init_func() fine
> with this? Basically, if we chardev_remove a device created by it (is this
> possible?) then the opts will be freed but it will remain inserted in its
> QemuOptsList.

Hmm? Don't see your issue here.
qemu_opts_del will also unlink from QemuOptsList ...

cheers,
  Gerd

On Thu, 18 Oct 2012 11:26:39 +0200
Gerd Hoffmann <kraxel@redhat.com> wrote:

>   Hi,
> 
> >> @@ -2864,6 +2868,7 @@ void qemu_chr_delete(CharDriverState *chr)
> >>          chr->chr_close(chr);
> >>      g_free(chr->filename);
> >>      g_free(chr->label);
> >> +    qemu_opts_del(chr->opts);
> >>      g_free(chr);
> >>  }
> > 
> > Didn't consider this in my first review, but is chardev_init_func() fine
> > with this? Basically, if we chardev_remove a device created by it (is this
> > possible?) then the opts will be freed but it will remain inserted in its
> > QemuOptsList.
> 
> Hmm? Don't see your issue here.
> qemu_opts_del will also unlink from QemuOptsList ...

Missed that fact, thanks for clarifying:

	Reviewed-by: Luiz Capitulino <lcapitulino@redhat.com>