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

[Alberto Garcia <berto@igalia.com>]

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.

> 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 :-?

>>+    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, and I'm not sure what
benefits using pathlib brings in this case.

>>+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.

>>+    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?

>>+sys.stdout.buffer.write(cluster)
>
> Would it be a good idea to check if stdout is a tty and not a
> pipe/redirection? You can check it with isatty() and error out to
> prevent printing binary to the terminal.

Yeah this is a good idea, thanks.

Berto