| Message ID | 20210211220146.2525771-2-crosa@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | Python / Acceptance Tests: improve logging | expand |
Hi, On 2/11/21 7:01 PM, Cleber Rosa wrote: > Closing a file that is open for writing, and then reading from it > sounds like a better idea than the opposite, given that the content > will be flushed. > > Reference: https://docs.python.org/3/library/io.html#io.IOBase.close > Signed-off-by: Cleber Rosa <crosa@redhat.com> > --- > python/qemu/machine.py | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/python/qemu/machine.py b/python/qemu/machine.py > index 7a40f4604b..6e44bda337 100644 > --- a/python/qemu/machine.py > +++ b/python/qemu/machine.py > @@ -337,12 +337,12 @@ class QEMUMachine: > self._qmp.close() > self._qmp_connection = None > > - self._load_io_log() > - > if self._qemu_log_file is not None: > self._qemu_log_file.close() > self._qemu_log_file = None > > + self._load_io_log() > + IMO it's a too fragile fix. It needs the `self._qemu_log_file.close()` being called before `self._load_io_log()` but a future change can make this condition unmet again. Maybe you could document that in the code. Or change the `_load_io_log()` to close the log file if it is open (also document it in code). - Wainer > self._qemu_log_path = None > > if self._temp_dir is not None:
On 2/11/21 5:01 PM, Cleber Rosa wrote: > Closing a file that is open for writing, and then reading from it > sounds like a better idea than the opposite, given that the content > will be flushed. > > Reference: https://docs.python.org/3/library/io.html#io.IOBase.close > Signed-off-by: Cleber Rosa <crosa@redhat.com> > --- > python/qemu/machine.py | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/python/qemu/machine.py b/python/qemu/machine.py > index 7a40f4604b..6e44bda337 100644 > --- a/python/qemu/machine.py > +++ b/python/qemu/machine.py > @@ -337,12 +337,12 @@ class QEMUMachine: Is there a way to improve context for python functions? What method is this in? etc. > self._qmp.close() > self._qmp_connection = None > > - self._load_io_log() > - > if self._qemu_log_file is not None: > self._qemu_log_file.close() > self._qemu_log_file = None > > + self._load_io_log() > + > self._qemu_log_path = None > > if self._temp_dir is not None: > Yeh, seems fine, though as wainer points out the interdependencies between _load_io_log, _qemu_log_file and _qemu_log_path are not all strictly clear, so it seems fragile. But, this is more correct than it was, so: Reviewed-by: John Snow <jsnow@redhat.com>
On 2/15/21 4:04 PM, John Snow wrote: > On 2/11/21 5:01 PM, Cleber Rosa wrote: >> Closing a file that is open for writing, and then reading from it >> sounds like a better idea than the opposite, given that the content >> will be flushed. >> >> Reference: https://docs.python.org/3/library/io.html#io.IOBase.close >> Signed-off-by: Cleber Rosa <crosa@redhat.com> >> --- >> python/qemu/machine.py | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/python/qemu/machine.py b/python/qemu/machine.py >> index 7a40f4604b..6e44bda337 100644 >> --- a/python/qemu/machine.py >> +++ b/python/qemu/machine.py >> @@ -337,12 +337,12 @@ class QEMUMachine: > > Is there a way to improve context for python functions? What method is > this in? etc. 'man gitattributes' suggests that having this line in .gitattributes would help: *.py diff=python /me goes to play with it... Does this look better to you? It certainly does to me! I'll go ahead and propose the .gitattributes change as a formal patch... --- a/python/qemu/machine.py +++ b/python/qemu/machine.py -@@ -337,12 +337,12 @@ class QEMUMachine: +@@ -337,12 +337,12 @@ def _post_shutdown(self) -> None: self._qmp.close()
On Mon, Feb 15, 2021 at 03:30:16PM -0300, Wainer dos Santos Moschetta wrote: > Hi, > > On 2/11/21 7:01 PM, Cleber Rosa wrote: > > Closing a file that is open for writing, and then reading from it > > sounds like a better idea than the opposite, given that the content > > will be flushed. > > > > Reference: https://docs.python.org/3/library/io.html#io.IOBase.close > > Signed-off-by: Cleber Rosa <crosa@redhat.com> > > --- > > python/qemu/machine.py | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/python/qemu/machine.py b/python/qemu/machine.py > > index 7a40f4604b..6e44bda337 100644 > > --- a/python/qemu/machine.py > > +++ b/python/qemu/machine.py > > @@ -337,12 +337,12 @@ class QEMUMachine: > > self._qmp.close() > > self._qmp_connection = None > > - self._load_io_log() > > - > > if self._qemu_log_file is not None: > > self._qemu_log_file.close() > > self._qemu_log_file = None > > + self._load_io_log() > > + > > > IMO it's a too fragile fix. It needs the `self._qemu_log_file.close()` being > called before `self._load_io_log()` but a future change can make this > condition unmet again. Maybe you could document that in the code. Or change > the `_load_io_log()` to close the log file if it is open (also document it > in code). > > - Wainer > I'm glad you see this is needed... and then something else. I'll investigate making this more robust as time allows it. Question is: do you ack/nack this change? Cheers, - Cleber.
On Mon, Feb 15, 2021 at 05:04:24PM -0500, John Snow wrote: > On 2/11/21 5:01 PM, Cleber Rosa wrote: > > Closing a file that is open for writing, and then reading from it > > sounds like a better idea than the opposite, given that the content > > will be flushed. > > > > Reference: https://docs.python.org/3/library/io.html#io.IOBase.close > > Signed-off-by: Cleber Rosa <crosa@redhat.com> > > --- > > python/qemu/machine.py | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/python/qemu/machine.py b/python/qemu/machine.py > > index 7a40f4604b..6e44bda337 100644 > > --- a/python/qemu/machine.py > > +++ b/python/qemu/machine.py > > @@ -337,12 +337,12 @@ class QEMUMachine: > > Is there a way to improve context for python functions? What method is this > in? etc. > > > self._qmp.close() > > self._qmp_connection = None > > - self._load_io_log() > > - > > if self._qemu_log_file is not None: > > self._qemu_log_file.close() > > self._qemu_log_file = None > > + self._load_io_log() > > + > > self._qemu_log_path = None > > if self._temp_dir is not None: > > > > Yeh, seems fine, though as wainer points out the interdependencies between > _load_io_log, _qemu_log_file and _qemu_log_path are not all strictly clear, > so it seems fragile. > Yep, agreed. This was a first, conservative change. Expect more later. > But, this is more correct than it was, so: > > Reviewed-by: John Snow <jsnow@redhat.com> Thanks, - Cleber
On 2/15/21 11:34 PM, Cleber Rosa wrote: > On Mon, Feb 15, 2021 at 03:30:16PM -0300, Wainer dos Santos Moschetta wrote: >> Hi, >> >> On 2/11/21 7:01 PM, Cleber Rosa wrote: >>> Closing a file that is open for writing, and then reading from it >>> sounds like a better idea than the opposite, given that the content >>> will be flushed. >>> >>> Reference: https://docs.python.org/3/library/io.html#io.IOBase.close >>> Signed-off-by: Cleber Rosa <crosa@redhat.com> >>> --- >>> python/qemu/machine.py | 4 ++-- >>> 1 file changed, 2 insertions(+), 2 deletions(-) >>> >>> diff --git a/python/qemu/machine.py b/python/qemu/machine.py >>> index 7a40f4604b..6e44bda337 100644 >>> --- a/python/qemu/machine.py >>> +++ b/python/qemu/machine.py >>> @@ -337,12 +337,12 @@ class QEMUMachine: >>> self._qmp.close() >>> self._qmp_connection = None >>> - self._load_io_log() >>> - >>> if self._qemu_log_file is not None: >>> self._qemu_log_file.close() >>> self._qemu_log_file = None >>> + self._load_io_log() >>> + >> >> IMO it's a too fragile fix. It needs the `self._qemu_log_file.close()` being >> called before `self._load_io_log()` but a future change can make this >> condition unmet again. Maybe you could document that in the code. Or change >> the `_load_io_log()` to close the log file if it is open (also document it >> in code). >> >> - Wainer >> > I'm glad you see this is needed... and then something else. I'll investigate > making this more robust as time allows it. > > Question is: do you ack/nack this change? hmm... /me thinking hmmm... okay, good deal. :) Acked-by: Wainer dos Santos Moschetta <wainersm@redhat.com> > > Cheers, > - Cleber.
diff --git a/python/qemu/machine.py b/python/qemu/machine.py index 7a40f4604b..6e44bda337 100644 --- a/python/qemu/machine.py +++ b/python/qemu/machine.py @@ -337,12 +337,12 @@ class QEMUMachine: self._qmp.close() self._qmp_connection = None - self._load_io_log() - if self._qemu_log_file is not None: self._qemu_log_file.close() self._qemu_log_file = None + self._load_io_log() + self._qemu_log_path = None if self._temp_dir is not None:
Closing a file that is open for writing, and then reading from it sounds like a better idea than the opposite, given that the content will be flushed. Reference: https://docs.python.org/3/library/io.html#io.IOBase.close Signed-off-by: Cleber Rosa <crosa@redhat.com> --- python/qemu/machine.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)