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

[Alberto Garcia <berto@igalia.com>]

On Mon 29 Jul 2024 03:20:18 PM -05, Eric Blake wrote:
> On Mon, Jul 29, 2024 at 05:02:26PM GMT, Alberto Garcia wrote:
>> +# qcow2 files produced by this script are always arranged like this:
>> +#
>> +# - qcow2 header
>> +# - refcount table
>> +# - refcount blocks
>> +# - L1 table
>> +# - L2 tables
>> +# - Data clusters
>
> Is it easy to make your tool spit out a qcow2 image with external data
> file (to write a quick qcow2 wrapper for an existing file to now be
> used as external data)?  Or is that too much of a difference from the
> intended use of this tool?

I didn't consider that use case and I didn't want to make the tool too
complicated. It's probably not very hard to do, if I can do it without
too many changes to the code I'll give it a try.

>> +def clusters_with_data(fd, cluster_size):
>> +    data_off = 0
>> +    while True:
>> +        hole_off = os.lseek(fd, data_off, os.SEEK_HOLE)
>> +        for idx in range(data_off // cluster_size, math.ceil(hole_off / cluster_size)):
>> +            yield idx
>> +        try:
>> +            data_off = os.lseek(fd, hole_off, os.SEEK_DATA)
>
> Depending on the size of cluster_size, this could return the same
> offset more than once (for example, for 1M clusters but 64k
> granularity on holes, consider what happens if lseek(0, SEEK_HOLE)
> returns 64k, then lseek(64k, SEEK_DATA) returns 128k: you end up
> yielding idx 0 twice).  You may need to be more careful than that.

I literally opened my laptop because I just realized that this could be
a problem :D I'll fix it in the next version.

>> +        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",
>> +            ],
>> +            capture_output=True,
>> +        )
>
> Does q-s-d exposing an image as raw still support lseek(SEEK_HOLE)
> efficiently?

Yes, I tested it before sending the new version, the improvements are
actually quite noticeable, thanks Nir for suggesting this.

>> +    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],
>
> Is it really worth trying to create v2 images?

I don't know, I added this because it required almost no changes to the
code. But I don't have a strong opinion, I can remove support for v2
images.

Berto