Re: [PATCH 03/20] python/machine.py: reorder __init__

[Kevin Wolf <kwolf@redhat.com>]

Am 07.10.2020 um 01:58 hat John Snow geschrieben:
> Put the init arg handling all at the top, and mostly in order (deviating
> when one is dependent on another), and put what is effectively runtime
> state declaration at the bottom.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  python/qemu/machine.py | 44 ++++++++++++++++++++++++------------------
>  1 file changed, 25 insertions(+), 19 deletions(-)
> 
> diff --git a/python/qemu/machine.py b/python/qemu/machine.py
> index 3017ec072df..71fe58be050 100644
> --- a/python/qemu/machine.py
> +++ b/python/qemu/machine.py
> @@ -84,42 +84,54 @@ def __init__(self, binary, args=None, wrapper=None, name=None,
>          @param monitor_address: address for QMP monitor
>          @param socket_scm_helper: helper program, required for send_fd_scm()
>          @param sock_dir: where to create socket (overrides test_dir for sock)
> -        @param console_log: (optional) path to console log file
>          @param drain_console: (optional) True to drain console socket to buffer
> +        @param console_log: (optional) path to console log file
>          @note: Qemu process is not started until launch() is used.
>          '''
> +        # Direct user configuration
> +
> +        self._binary = binary
> +
>          if args is None:
>              args = []
> +        # Copy mutable input: we will be modifying our copy
> +        self._args = list(args)
> +
>          if wrapper is None:
>              wrapper = []
> -        if name is None:
> -            name = "qemu-%d" % os.getpid()
> -        if sock_dir is None:
> -            sock_dir = test_dir
> -        self._name = name
> +        self._wrapper = wrapper
> +
> +        self._name = name or "qemu-%d" % os.getpid()
> +        self._test_dir = test_dir
> +        self._sock_dir = sock_dir or self._test_dir
> +        self._socket_scm_helper = socket_scm_helper

Interesting that you use a shortcut with 'or' for name and sock_dir,
but you don't have 'wraper or []' and 'args or []' above.

It's not wrong, of course, but if you have to respin for another reason,
maybe an inconsistency to address.

Reviewed-by: Kevin Wolf <kwolf@redhat.com>