Re: [PATCH] scripts/qcow2-to-stdout.py: Add script to write qcow2 images to stdout
On Mon 01 Jul 2024 02:07:01 PM +03, Manos Pitsidianakis wrote: >> and I'm not sure what benefits using pathlib brings in this case. > > implicit type requirement, argument value validations, path > normalization etc. Do you have a specific example? I don't see any difference in behavior if I make input_file a pathlib.Path, I still need to check if the file exists, etc., I don't see that this is validating anything. >> 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. In general I would agree (that's why I'm opening files this way in other parts of the script) but for this case I don't think it's worth it. >> 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. Ah I see. I'll send a new version soon. Thanks for the review, Berto
Re: [PATCH] scripts/qcow2-to-stdout.py: Add script to write qcow2 images to stdout
Hi Berto :)
On Mon, 1 Jul 2024 at 11:56, Alberto Garcia 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
Re: [PATCH] scripts/qcow2-to-stdout.py: Add script to write qcow2 images to stdout
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
Re: [PATCH] scripts/qcow2-to-stdout.py: Add script to write qcow2 images to stdout
On Wed, 12 Jun 2024 12:21, Alberto Garcia wrote: On Wed 12 Jun 2024 09:01:01 AM +03, Manos Pitsidianakis wrote: Hello Alberto, Hello Manos! > This is equivalent to using qemu-img to convert a file to qcow2 and > then writing the result to stdout, with the difference that this > tool does not need to create this temporary qcow2 file and therefore > does not need any additional disk space. Can you expand on this a little bit? Would modifying qemu-img to write to stdout if given, say, - instead of a file output path be enough to make this script unnecessary? Yes, it would be enough. Allowing qemu-img convert to write to stdout would indeed be very nice for the end user but it's a bit of a niche use case and it's also not a trivial task so I don't think that it's worth the effort. The output files that you pass to qemu-img convert need to be seekable because the only way to produce a qcow2 file without doing that is by precalculating all the metadata in advance before starting to write anything (that's why this script reads the input file twice). This is fundamentally different to what qemu-img convert does, which is to read the input file from start to finish and write it to the output file, relying on the relevant driver's existing write operations. All those assume random access to the output file. qemu-img is also much more generic in the sense that it supports many different output formats and image options. In contrast, writing the algorithm for a basic subset of qcow2 is quite simple and that's why I think that it makes sense to do it in a separate tool. Berto Thanks for the complete explanation! It makes sense. Maybe add it to the commit message as well, it's informative. Manos
Re: [PATCH] scripts/qcow2-to-stdout.py: Add script to write qcow2 images to stdout
On Mon, 10 Jun 2024 17:47, Alberto Garcia wrote:
This tool converts a disk image to qcow2, writing the result directly
to stdout. This can be used for example to send the generated file
over the network.
This is equivalent to using qemu-img to convert a file to qcow2 and
then writing the result to stdout, with the difference that this tool
does not need to create this temporary qcow2 file and therefore does
not need any additional disk space.
The input file is read twice. The first pass is used to determine
which clusters contain non-zero data and that information is used to
create the qcow2 header, refcount table and blocks, and L1 and L2
tables. After all that metadata is created then the second pass is
used to write the guest data.
By default qcow2-to-stdout.py expects the input to be a raw file, but
if qemu-storage-daemon is available then it can also be used to read
images in other formats. Alternatively the user can also run qemu-ndb
or qemu-storage-daemon manually instead.
Signed-off-by: Alberto Garcia
Signed-off-by: Madeeha Javed
---
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.
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
diff --git a/scripts/qcow2-to-stdout.py b/scripts/qcow2-to-stdout.py
new file mode 100755
index 00..b9f75de690
--- /dev/null
+++ b/scripts/qcow2-to-stdout.py
@@ -0,0 +1,330 @@
+#!/usr/bin/env python3
+
+# This tool reads a disk image in any format and converts it to qcow2,
+# writing the result directly to stdout.
+#
+# Copyright (C) 2024 Igalia, S.L.
+#
+# Authors: Alberto Garcia
+# Madeeha Javed
+#
+# SPDX-License-Identifier: GPL-2.0-or-later
+#
+# qcow2 files produced by this script are always arranged like this:
+#
+# - qcow2 header
+# - refcount table
+# - refcount blocks
+# - L1 table
+# - L2 tables
+# - Data clusters
+#
+# A note about variable names: in qcow2 there is one refcount table
+# and one (active) L1 table, although each can occupy several
+# clusters. For the sake of simplicity the code sometimes talks about
+# refcount tables and L1 tables when referring to those clusters.
+
+import argparse
+import atexit
+import math
+import os
+import signal
+import struct
+import sys
+import subprocess
+import tempfile
+import time
+
+QCOW2_DEFAULT_CLUSTER_SIZE = 65536
+QCOW2_DEFAULT_REFCOUNT_BITS = 16
+QCOW2_DEFAULT_VERSION = 3
+QCOW_OFLAG_COPIED = 1 << 63
+
+def bitmap_set(bitmap, idx):
+bitmap[int(idx / 8)] |= (1 << (idx % 8))
+
+def bitmap_test(bitmap, idx):
+return (bitmap[int(idx / 8)] & (1 << (idx % 8))) != 0
+
+# Kill the storage daemon on exit
+def kill_storage_daemon(pid_file, raw_file, temp_dir):
+if os.path.exists(pid_file):
+with open(pid_file, 'r') as f:
+pid=int(f.readline())
+os.kill(pid, signal.SIGTERM)
+while os.path.exists(pid_file):
+time.sleep(0.1)
+os.unlink(raw_file)
+os.rmdir(temp_dir)
+
+def write_features(header):
+qcow2_features = [
+# Incompatible
+(0, 0, 'dirty bit'),
+(0, 1, 'corrupt bit'),
+(0, 2, 'external data file'),
+(0, 3, 'compression type'),
+(0, 4, 'extended L2 entries'),
+# Compatible
+(1, 0, 'lazy refcounts'),
+# Autoclear
+(2, 0, 'bitmaps'),
+(2, 1, 'raw external data')
+]
+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.
+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:
if __name__ == "__main__":
parser = argparse.ArgumentParser(
description="This program converts a QEMU disk image to qcow2 "
"and writes it to the standard output"
)
etc...
+# 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)
+parser.add_argument('-
Re: [PATCH] scripts/qcow2-to-stdout.py: Add script to write qcow2 images to stdout
On Wed 12 Jun 2024 09:01:01 AM +03, Manos Pitsidianakis wrote: > Hello Alberto, Hello Manos! > > This is equivalent to using qemu-img to convert a file to qcow2 and > > then writing the result to stdout, with the difference that this > > tool does not need to create this temporary qcow2 file and therefore > > does not need any additional disk space. > > Can you expand on this a little bit? Would modifying qemu-img to write > to stdout if given, say, - instead of a file output path be enough to > make this script unnecessary? Yes, it would be enough. Allowing qemu-img convert to write to stdout would indeed be very nice for the end user but it's a bit of a niche use case and it's also not a trivial task so I don't think that it's worth the effort. The output files that you pass to qemu-img convert need to be seekable because the only way to produce a qcow2 file without doing that is by precalculating all the metadata in advance before starting to write anything (that's why this script reads the input file twice). This is fundamentally different to what qemu-img convert does, which is to read the input file from start to finish and write it to the output file, relying on the relevant driver's existing write operations. All those assume random access to the output file. qemu-img is also much more generic in the sense that it supports many different output formats and image options. In contrast, writing the algorithm for a basic subset of qcow2 is quite simple and that's why I think that it makes sense to do it in a separate tool. Berto
Re: [PATCH] scripts/qcow2-to-stdout.py: Add script to write qcow2 images to stdout
Hello Alberto, On Mon, 10 Jun 2024 17:47, Alberto Garcia wrote: This tool converts a disk image to qcow2, writing the result directly to stdout. This can be used for example to send the generated file over the network. This is equivalent to using qemu-img to convert a file to qcow2 and then writing the result to stdout, with the difference that this tool does not need to create this temporary qcow2 file and therefore does not need any additional disk space. Can you expand on this a little bit? Would modifying qemu-img to write to stdout if given, say, - instead of a file output path be enough to make this script unnecessary? The input file is read twice. The first pass is used to determine which clusters contain non-zero data and that information is used to create the qcow2 header, refcount table and blocks, and L1 and L2 tables. After all that metadata is created then the second pass is used to write the guest data. By default qcow2-to-stdout.py expects the input to be a raw file, but if qemu-storage-daemon is available then it can also be used to read images in other formats. Alternatively the user can also run qemu-ndb or qemu-storage-daemon manually instead. Signed-off-by: Alberto Garcia Signed-off-by: Madeeha Javed ---
[PATCH] scripts/qcow2-to-stdout.py: Add script to write qcow2 images to stdout
This tool converts a disk image to qcow2, writing the result directly
to stdout. This can be used for example to send the generated file
over the network.
This is equivalent to using qemu-img to convert a file to qcow2 and
then writing the result to stdout, with the difference that this tool
does not need to create this temporary qcow2 file and therefore does
not need any additional disk space.
The input file is read twice. The first pass is used to determine
which clusters contain non-zero data and that information is used to
create the qcow2 header, refcount table and blocks, and L1 and L2
tables. After all that metadata is created then the second pass is
used to write the guest data.
By default qcow2-to-stdout.py expects the input to be a raw file, but
if qemu-storage-daemon is available then it can also be used to read
images in other formats. Alternatively the user can also run qemu-ndb
or qemu-storage-daemon manually instead.
Signed-off-by: Alberto Garcia
Signed-off-by: Madeeha Javed
---
scripts/qcow2-to-stdout.py | 330 +
1 file changed, 330 insertions(+)
create mode 100755 scripts/qcow2-to-stdout.py
diff --git a/scripts/qcow2-to-stdout.py b/scripts/qcow2-to-stdout.py
new file mode 100755
index 00..b9f75de690
--- /dev/null
+++ b/scripts/qcow2-to-stdout.py
@@ -0,0 +1,330 @@
+#!/usr/bin/env python3
+
+# This tool reads a disk image in any format and converts it to qcow2,
+# writing the result directly to stdout.
+#
+# Copyright (C) 2024 Igalia, S.L.
+#
+# Authors: Alberto Garcia
+# Madeeha Javed
+#
+# SPDX-License-Identifier: GPL-2.0-or-later
+#
+# qcow2 files produced by this script are always arranged like this:
+#
+# - qcow2 header
+# - refcount table
+# - refcount blocks
+# - L1 table
+# - L2 tables
+# - Data clusters
+#
+# A note about variable names: in qcow2 there is one refcount table
+# and one (active) L1 table, although each can occupy several
+# clusters. For the sake of simplicity the code sometimes talks about
+# refcount tables and L1 tables when referring to those clusters.
+
+import argparse
+import atexit
+import math
+import os
+import signal
+import struct
+import sys
+import subprocess
+import tempfile
+import time
+
+QCOW2_DEFAULT_CLUSTER_SIZE = 65536
+QCOW2_DEFAULT_REFCOUNT_BITS = 16
+QCOW2_DEFAULT_VERSION = 3
+QCOW_OFLAG_COPIED = 1 << 63
+
+def bitmap_set(bitmap, idx):
+bitmap[int(idx / 8)] |= (1 << (idx % 8))
+
+def bitmap_test(bitmap, idx):
+return (bitmap[int(idx / 8)] & (1 << (idx % 8))) != 0
+
+# Kill the storage daemon on exit
+def kill_storage_daemon(pid_file, raw_file, temp_dir):
+if os.path.exists(pid_file):
+with open(pid_file, 'r') as f:
+pid=int(f.readline())
+os.kill(pid, signal.SIGTERM)
+while os.path.exists(pid_file):
+time.sleep(0.1)
+os.unlink(raw_file)
+os.rmdir(temp_dir)
+
+def write_features(header):
+qcow2_features = [
+# Incompatible
+(0, 0, 'dirty bit'),
+(0, 1, 'corrupt bit'),
+(0, 2, 'external data file'),
+(0, 3, 'compression type'),
+(0, 4, 'extended L2 entries'),
+# Compatible
+(1, 0, 'lazy refcounts'),
+# Autoclear
+(2, 0, 'bitmaps'),
+(2, 1, 'raw external data')
+]
+struct.pack_into('>I', header, 0x70, 0x6803f857)
+struct.pack_into('>I', header, 0x74, len(qcow2_features) * 48)
+cur_offset = 0x78
+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
+
+# 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')
+parser.add_argument('-f', dest='input_format', metavar='input_format',
+default='raw',
+help='format of the input file (default: raw)')
+parser.add_argument('-c', dest='cluster_size', metavar='cluster_size',
+help=f'qcow2 cluster size (default:
{QCOW2_DEFAULT_CLUSTER_SIZE})',
+default=QCOW2_DEFAULT_CLUSTER_SIZE, type=int,
+choices=[1 << x for x in range(9,22)])
+parser.add_argument('-r', dest='refcount_bits', metavar='refcount_bits',
+help=f'width of the reference count entries (default:
{QCOW2_DEFAULT_REFCOUNT_BITS})',
+default=QCOW2_DEFAULT_REFCOUNT_BITS, type=int,
+choices=[1 << x for x in range(7)])
+parser.add_argument('-v', dest='qcow2_version', metavar='qcow2_version',
+help=f'qcow2 version (default: {QCOW2_DEFAULT_VERSION})',
+default=QCOW2_DEFAULT_VERSION, type=int, choices=[2, 3])
+args = parser.parse_args()
+
+qcow2_cl
