diff mbox series

[1/6] Python: close the log file kept by QEMUMachine before reading it

Message ID 20210211220146.2525771-2-crosa@redhat.com (mailing list archive)
State New, archived
Headers show
Series Python / Acceptance Tests: improve logging | expand

Commit Message

Cleber Rosa Feb. 11, 2021, 10:01 p.m. UTC
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(-)

Comments

Wainer dos Santos Moschetta Feb. 15, 2021, 6:30 p.m. UTC | #1
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:
John Snow Feb. 15, 2021, 10:04 p.m. UTC | #2
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>
Eric Blake Feb. 15, 2021, 10:19 p.m. UTC | #3
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()
Cleber Rosa Feb. 16, 2021, 2:34 a.m. UTC | #4
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.
Cleber Rosa Feb. 16, 2021, 2:35 a.m. UTC | #5
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
Wainer dos Santos Moschetta Feb. 17, 2021, 7:53 p.m. UTC | #6
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 mbox series

Patch

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: