| Message ID | 20201006235817.3280413-4-jsnow@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | python/qemu: strictly typed mypy conversion, pt2 | expand |
On 10/7/20 1:58 AM, John Snow wrote: > 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(-) Reviewed-by: Philippe Mathieu-Daudé <philmd@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>
On 10/7/20 5:43 AM, Kevin Wolf wrote: > 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. > This winds up being because ... I delete those lines of code later. I very often have this problem where I clean up a bunch of stuff, and then split out that giant commit into a series. Sometimes that causes stuff like this. > Reviewed-by: Kevin Wolf <kwolf@redhat.com> > Thanks!
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 + if monitor_address is not None: self._monitor_address = monitor_address self._remove_monitor_sockfile = False else: self._monitor_address = os.path.join( - sock_dir, f"{name}-monitor.sock" + self._sock_dir, f"{self._name}-monitor.sock" ) self._remove_monitor_sockfile = True + + self._console_log_path = console_log + if self._console_log_path: + # In order to log the console, buffering needs to be enabled. + self._drain_console = True + else: + self._drain_console = drain_console + + # Runstate self._qemu_log_path = None self._qemu_log_file = None self._popen = None - self._binary = binary - self._args = list(args) # Force copy args in case we modify them - self._wrapper = wrapper self._events = [] self._iolog = None - self._socket_scm_helper = socket_scm_helper self._qmp_set = True # Enable QMP monitor by default. self._qmp = None self._qemu_full_args = None - self._test_dir = test_dir self._temp_dir = None - self._sock_dir = sock_dir self._launched = False self._machine = None self._console_index = 0 @@ -129,12 +141,6 @@ def __init__(self, binary, args=None, wrapper=None, name=None, self._console_socket = None self._remove_files = [] self._user_killed = False - self._console_log_path = console_log - if self._console_log_path: - # In order to log the console, buffering needs to be enabled. - self._drain_console = True - else: - self._drain_console = drain_console def __enter__(self): return self
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(-)