qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v2 28/38] qapi/gen.py: update write() to be more idiomatic


From: John Snow
Subject: Re: [PATCH v2 28/38] qapi/gen.py: update write() to be more idiomatic
Date: Fri, 25 Sep 2020 12:33:37 -0400
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.11.0

On 9/25/20 9:26 AM, Eduardo Habkost wrote:
On Fri, Sep 25, 2020 at 03:15:57PM +0200, Markus Armbruster wrote:
Cleber Rosa <crosa@redhat.com> writes:

On Wed, Sep 23, 2020 at 02:37:27PM -0400, John Snow wrote:
On 9/23/20 11:26 AM, Eduardo Habkost wrote:
On Tue, Sep 22, 2020 at 05:00:51PM -0400, John Snow wrote:
Make the file handling here just a tiny bit more idiomatic.
(I realize this is heavily subjective.)

Use exist_ok=True for os.makedirs and remove the exception,
use fdopen() to wrap the file descriptor in a File-like object,
and use a context manager for managing the file pointer.

Signed-off-by: John Snow <jsnow@redhat.com>

Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>

I really miss a comment below explaining why we use
open(os.open(pathname, ...), ...) instead of open(pathname, ...).

This code:

         fd = os.open(pathname, os.O_RDWR | os.O_CREAT, 0o666)
         f = open(fd, 'r+', encoding='utf-8')

Not known to me. It was introduced in 907b846653 as part of an effort to
reduce rebuild times. Maybe this avoids a modification time change if the
file already exists?

Markus?

AFACIT the change on 907b846653 is effective because of the "is new
text different from old text?" conditional.  I can not see how the
separate/duplicate open/fdopen would contribute to that.

But, let's hear from Markus.

This was my best attempt to open the file read/write, creating it if it
doesn't exist.

Plain

         f = open(pathname, "r+", encoding='utf-8')

fails instead of creates, and

         f = open(pathname, "w+", encoding='utf-8')

truncates.

If you know a better way, tell me!

Thanks for the explanation!

Yeah, it looks like there's no combination of open() flags that
would get translated to O_RDWR|O_CREAT.

Using os.open() like you did seems more straightforward than
catching FileNotFoundError.


OK. Using fdopen works for us, so let's stick with it. I will add a comment explaining our reasoning.

Thanks!

--js




reply via email to

[Prev in Thread] Current Thread [Next in Thread]