Re: [Qemu-devel] [PATCH v2 04/19] chardev: Shorten references into ChardevBackend

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> An upcoming patch will alter how simple unions, like ChardevBackend,
> are laid out, which will impact all lines of the form 'backend->u.XXX'.
> To minimize the impact of that patch, use a temporary variable to
> reduce the number of lines needing modification when an internal
> reference within ChardevBackend changes layout.
>
> Signed-off-by: Eric Blake <address@hidden>
> Reviewed-By: Daniel P. Berrange <address@hidden>
>
> ---
> v2: add R-b
> ---
>  qemu-char.c | 122 
> ++++++++++++++++++++++++++++++++----------------------------
>  1 file changed, 66 insertions(+), 56 deletions(-)
>
> diff --git a/qemu-char.c b/qemu-char.c
> index fc8ffda..5ea1d34 100644
> --- a/qemu-char.c
> +++ b/qemu-char.c
> @@ -724,7 +724,7 @@ static CharDriverState *qemu_chr_open_mux(const char *id,
>      ChardevMux *mux = backend->u.mux;
>      CharDriverState *chr, *drv;
>      MuxDriver *d;
> -    ChardevCommon *common = qapi_ChardevMux_base(backend->u.mux);
> +    ChardevCommon *common = qapi_ChardevMux_base(mux);
>
>      drv = qemu_chr_find(mux->chardev);
>      if (drv == NULL) {

The commit message sounds like you *add* a temporary variable to reduce
churn.  You're using an existing one here.

> @@ -1043,7 +1043,7 @@ static CharDriverState *qemu_chr_open_pipe(const char 
> *id,
>      char *filename_in;
>      char *filename_out;
>      const char *filename = opts->device;
> -    ChardevCommon *common = qapi_ChardevHostdev_base(backend->u.pipe);
> +    ChardevCommon *common = qapi_ChardevHostdev_base(opts);
>
>
>      filename_in = g_strdup_printf("%s.in", filename);
> @@ -1123,7 +1123,7 @@ static CharDriverState *qemu_chr_open_stdio(const char 
> *id,
>      ChardevStdio *opts = backend->u.stdio;
>      CharDriverState *chr;
>      struct sigaction act;
> -    ChardevCommon *common = qapi_ChardevStdio_base(backend->u.stdio);
> +    ChardevCommon *common = qapi_ChardevStdio_base(opts);
>
>      if (is_daemonized()) {
>          error_setg(errp, "cannot use stdio with -daemonize");
> @@ -2141,7 +2141,7 @@ static CharDriverState *qemu_chr_open_pipe(const char 
> *id,
>      const char *filename = opts->device;
>      CharDriverState *chr;
>      WinCharState *s;
> -    ChardevCommon *common = qapi_ChardevHostdev_base(backend->u.pipe);
> +    ChardevCommon *common = qapi_ChardevHostdev_base(opts);
>
>      chr = qemu_chr_alloc(common, errp);
>      if (!chr) {
> @@ -3216,7 +3216,7 @@ static CharDriverState *qemu_chr_open_ringbuf(const 
> char *id,
>                                                Error **errp)
>  {
>      ChardevRingbuf *opts = backend->u.ringbuf;
> -    ChardevCommon *common = qapi_ChardevRingbuf_base(backend->u.ringbuf);
> +    ChardevCommon *common = qapi_ChardevRingbuf_base(opts);
>      CharDriverState *chr;
>      RingBufCharDriver *d;
>
> @@ -3506,26 +3506,29 @@ static void qemu_chr_parse_file_out(QemuOpts *opts, 
> ChardevBackend *backend,
>                                      Error **errp)
>  {
>      const char *path = qemu_opt_get(opts, "path");
> +    ChardevFile *file;

Ah, you do add a temporary in some places.

>
>      if (path == NULL) {
>          error_setg(errp, "chardev: file: no filename given");
>          return;
>      }
> -    backend->u.file = g_new0(ChardevFile, 1);
> -    qemu_chr_parse_common(opts, qapi_ChardevFile_base(backend->u.file));
> -    backend->u.file->out = g_strdup(path);
> +    file = backend->u.file = g_new0(ChardevFile, 1);
> +    qemu_chr_parse_common(opts, qapi_ChardevFile_base(file));
> +    file->out = g_strdup(path);
>
> -    backend->u.file->has_append = true;
> -    backend->u.file->append = qemu_opt_get_bool(opts, "append", false);
> +    file->has_append = true;
> +    file->append = qemu_opt_get_bool(opts, "append", false);
>  }


Whether you touch every line now or later is a wash as far as churn is
concerned.  I'd be willing to accept an argument that this change is
simpler than the one it avoids, or that it makes the code more
consistent, or it makes the code easier to read.  Preferably in the
commit message.

[More of the same snipped...]