Re: [PATCH] scripts/qcow2-to-stdout.py: Add script to write qcow2 images to stdout

[Manos Pitsidianakis]

Hi Berto :)

On Mon, 1 Jul 2024 at 11:56, Alberto Garcia <berto@igalia.com> wrote:
>
> On Wed 12 Jun 2024 02:00:19 PM +03, Manos Pitsidianakis wrote:
>
> Hi, thanks for the review and sorry for taking so long to reply, I was
> on vacation.
>
> >> scripts/qcow2-to-stdout.py | 330 +++++++++++++++++++++++++++++++++++++
> >> 1 file changed, 330 insertions(+)
> >> create mode 100755 scripts/qcow2-to-stdout.py
> >
> > I recommend running the `black` formatter on this script, it makes the
> > code more diff-friendly and uniform. Also it has become the de-facto
> > python style.
>
> Hmmm, I don't like how it reformats some of the lines. However other
> changes do make sense, so I'll apply those.

It's not a project coding style requirement (for now) so it's fine.

>
> > Also, it's more pythonic to name constants in uppercase, like
> > allocated_l2_tables. You can check such lints with pylint
> > scripts/qcow2-to-stdout.py
>
> allocated_l2_tables is not a constant :-?

Eeeh right, correct. `pylint`'s error message said it was a constant,
my bad. It says it is a constant because it is declared as a global
(module-level), `__all__` is not defined with any globals, and
according to PEP-8 non-public globals start with an underscore in the
name.

>
> >>+    struct.pack_into('>I', header, 0x70, 0x6803f857)
> >>+    struct.pack_into('>I', header, 0x74, len(qcow2_features) * 48)
> >>+    cur_offset = 0x78
> >
> > Minor comment: extract magic values/offsets into constant globals with
> > descriptive names, it'd help the code be more readable and easier to
> > maintain if ported in the future to other formats.
>
> Good idea, will do.
>
> >>+    for (feature_type, feature_bit, feature_name) in qcow2_features:
> >>+        struct.pack_into('>BB46s', header, cur_offset,
> >>+                         feature_type, feature_bit, 
> >>feature_name.encode('ascii'))
> >>+        cur_offset += 48
> >>+
> >
> >>From here onwards put everything under a main block like so:
>
> Ok.
>
> >>+# Command-line arguments
> >>+parser = argparse.ArgumentParser(description='This program converts a QEMU 
> >>disk image to qcow2 '
> >>+                                 'and writes it to the standard output')
> >>+parser.add_argument('input_file', help='name of the input file')
> >
> > Suggestion:
> >
> > -parser.add_argument('input_file', help='name of the input file')
> > +parser.add_argument('input_file', help='name of the input file', 
> > type=pathlib.Path, required=True)
>
> 'required' is not valid in positional arguments,

Sorry did not notice it's a positional!

> and I'm not sure what
> benefits using pathlib brings in this case.

implicit type requirement, argument value validations, path normalization etc.

>
> >>+parser.add_argument('-v', dest='qcow2_version', metavar='qcow2_version',
> >
> > Maybe -q instead of -v? No strong feelings on this one, it's just that
> > -v is usually version. -q is also usually --quiet so not sure...
>
> Yeah, I thought the same but I didn't want to complicate this too much,
> this is just a helper script.
>
> >>+# If the input file is not in raw format we can use
> >>+# qemu-storage-daemon to make it appear as such
> >>+if input_format != 'raw':
> >>+    temp_dir = tempfile.mkdtemp()
> >
> > Consider using the tempfile.TemporaryDirectory as with... context
> > manager so that the temp dir cleanup is performed automatically
>
> I don't think I can do that directly here because the temp dir has to
> live until the very end (qemu-storage-daemon needs it).
>
> >>+    pid_file = temp_dir + "/pid"
> >>+    raw_file = temp_dir + "/raw"
> >>+    open(raw_file, 'wb').close()
> >
> > Consider using a with open(...) open manager for opening the file
>
> How would that be? Like this?
>
>     with open(raw_file, 'wb'):
>         pass
>
> If so I don't see the benefit, I just need to create an empty file and
> close it immediately.

My only argument here is that it's "more pythonic" which I know is of
little value and consequence :) Feel free to ignore! They were mere
suggestions.

>
> >>+    atexit.register(kill_storage_daemon, pid_file, raw_file, temp_dir)
> >
> > Hm, this too could be a context manager. Seems very C-like to use
> > atexit here.
>
> Yeah it is, but I think that using the context manager would require me
> to split the main function in two, and I'm not sure that it's worth it
> for this case. Other Python scripts in the QEMU repo use atexit already.
>
> >>+    ret = subprocess.run(["qemu-storage-daemon", "--daemonize", 
> >>"--pidfile", pid_file,
> >>+                          "--blockdev", 
> >>f"driver=file,node-name=file0,driver=file,filename={input_file},read-only=on",
> >>+                          "--blockdev", 
> >>f"driver={input_format},node-name=disk0,file=file0,read-only=on",
> >>+                          "--export", 
> >>f"type=fuse,id=export0,node-name=disk0,mountpoint={raw_file},writable=off"])
> >
> > You can add shell=True, check=False arguments to subprocess.run() so
> > that it captures the outputs. (check=False is the default behavior, but
> > better make it explicit)
>
> I'm not sure that I understand, why would I need to use a shell here?

I must have meant capture_output=True, not shell=True, sorry for that
🤔. The explicit check=False says to the reader that this won't throw
an exception so it's just for readability. The capture_output part is
so that you can print the outputs if the return code is an error.

I wouldn't want to overcomplicate things for no real reason,
especially for a simple single patch like this so I suggested minor
stuff for code readability. Should have made it more explicit in the
first place! :)

-- 
Manos Pitsidianakis
Emulation and Virtualization Engineer at Linaro Ltd