Re: [PATCH v3 RESEND 2/2] chardev: refactor qmp_chardev_add and qmp_chardev_change

[Li Zhang]

Hi all,

Any comments for this patch?

On Fri, Jul 9, 2021 at 6:11 PM Li Zhang <zhlcindy@gmail.com> wrote:
>
> To improve the problematic source code in qmp_chardev_change
> and some redundant source code, the functions qmp_chardev_add
> and qmp_chardev_change are refactored. It is mentioned in thread:
> https://lists.nongnu.org/archive/html/qemu-devel/2021-04/msg03167.html
>
> Signed-off-by: Li Zhang <li.zhang@ionos.com>
> ---
>  chardev/char.c | 72 ++++++++++++++++++++++++++------------------------
>  1 file changed, 37 insertions(+), 35 deletions(-)
>
> diff --git a/chardev/char.c b/chardev/char.c
> index d959eec522..cb6f287cd2 100644
> --- a/chardev/char.c
> +++ b/chardev/char.c
> @@ -1028,23 +1028,10 @@ Chardev *qemu_chardev_new(const char *id, const char 
> *typename,
>      return chr;
>  }
>
> -ChardevReturn *qmp_chardev_add(const char *id, ChardevBackend *backend,
> -                               Error **errp)
> +static ChardevReturn *chardev_add(const char *id, Chardev *chr,
> +                                  Error **errp)
>  {
> -    const ChardevClass *cc;
>      ChardevReturn *ret;
> -    Chardev *chr;
> -
> -    cc = char_get_class(ChardevBackendKind_str(backend->type), errp);
> -    if (!cc) {
> -        return NULL;
> -    }
> -
> -    chr = chardev_new(id, object_class_get_name(OBJECT_CLASS(cc)),
> -                      backend, NULL, false, errp);
> -    if (!chr) {
> -        return NULL;
> -    }
>
>      if (!object_property_try_add_child(get_chardevs_root(), id, OBJECT(chr),
>                                         errp)) {
> @@ -1062,6 +1049,26 @@ ChardevReturn *qmp_chardev_add(const char *id, 
> ChardevBackend *backend,
>      return ret;
>  }
>
> +ChardevReturn *qmp_chardev_add(const char *id, ChardevBackend *backend,
> +                               Error **errp)
> +{
> +    const ChardevClass *cc;
> +    Chardev *chr;
> +
> +    cc = char_get_class(ChardevBackendKind_str(backend->type), errp);
> +    if (!cc) {
> +        return NULL;
> +    }
> +
> +    chr = chardev_new(id, object_class_get_name(OBJECT_CLASS(cc)),
> +                      backend, NULL, false, errp);
> +    if (!chr) {
> +        return NULL;
> +    }
> +
> +    return chardev_add(id, chr, errp);
> +}
> +
>  ChardevReturn *qmp_chardev_change(const char *id, ChardevBackend *backend,
>                                    Error **errp)
>  {
> @@ -1070,7 +1077,6 @@ ChardevReturn *qmp_chardev_change(const char *id, 
> ChardevBackend *backend,
>      Chardev *chr, *chr_new;
>      bool closed_sent = false;
>      bool handover_yank_instance;
> -    ChardevReturn *ret;
>
>      chr = qemu_chr_find(id);
>      if (!chr) {
> @@ -1089,11 +1095,22 @@ ChardevReturn *qmp_chardev_change(const char *id, 
> ChardevBackend *backend,
>          return NULL;
>      }
>
> +    cc = CHARDEV_GET_CLASS(chr);
> +    cc_new = char_get_class(ChardevBackendKind_str(backend->type), errp);
> +    if (!cc_new) {
> +        return NULL;
> +    }
> +
>      be = chr->be;
>      if (!be) {
>          /* easy case */
> -        object_unparent(OBJECT(chr));
> -        return qmp_chardev_add(id, backend, errp);
> +        chr_new = chardev_new(id, 
> object_class_get_name(OBJECT_CLASS(cc_new)),
> +                              backend, NULL, false, errp);
> +        if (!chr_new) {
> +            return NULL;
> +        }
> +
> +        goto out;
>      }
>
>      if (!be->chr_be_change) {
> @@ -1101,12 +1118,6 @@ ChardevReturn *qmp_chardev_change(const char *id, 
> ChardevBackend *backend,
>          return NULL;
>      }
>
> -    cc = CHARDEV_GET_CLASS(chr);
> -    cc_new = char_get_class(ChardevBackendKind_str(backend->type), errp);
> -    if (!cc_new) {
> -        return NULL;
> -    }
> -
>      /*
>       * The new chardev should not register a yank instance if the current
>       * chardev has registered one already.
> @@ -1147,18 +1158,9 @@ ChardevReturn *qmp_chardev_change(const char *id, 
> ChardevBackend *backend,
>       */
>      chr->handover_yank_instance = handover_yank_instance;
>
> +out:
>      object_unparent(OBJECT(chr));
> -    object_property_add_child(get_chardevs_root(), chr_new->label,
> -                              OBJECT(chr_new));
> -    object_unref(OBJECT(chr_new));
> -
> -    ret = g_new0(ChardevReturn, 1);
> -    if (CHARDEV_IS_PTY(chr_new)) {
> -        ret->pty = g_strdup(chr_new->filename + 4);
> -        ret->has_pty = true;
> -    }
> -
> -    return ret;
> +    return chardev_add(id, chr_new, errp);
>  }
>
>  void qmp_chardev_remove(const char *id, Error **errp)
> --
> 2.25.1
>


-- 

Best Regards
-Li