[PATCH] qemu-nbd: Removed deprecated --partition option

[PATCH] qemu-nbd: Removed deprecated --partition option

[Eric Blake]

The option was deprecated in 4.0.0 (commit 0ae2d546); it's now been
long enough with no complaints to follow through with that process.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 qemu-deprecated.texi |  49 ++++++----------
 qemu-nbd.c           | 133 +------------------------------------------
 qemu-nbd.texi        |  13 ++---
 3 files changed, 24 insertions(+), 171 deletions(-)

diff --git a/qemu-deprecated.texi b/qemu-deprecated.texi
index 8471eef9c22d..1b4c638db8e0 100644
--- a/qemu-deprecated.texi
+++ b/qemu-deprecated.texi
@@ -304,37 +304,6 @@ The above, converted to the current supported format:

 @section Related binaries

-@subsection qemu-nbd --partition (since 4.0.0)
-
-The ``qemu-nbd --partition $digit'' code (also spelled @option{-P})
-can only handle MBR partitions, and has never correctly handled
-logical partitions beyond partition 5.  If you know the offset and
-length of the partition (perhaps by using @code{sfdisk} within the
-guest), you can achieve the effect of exporting just that subset of
-the disk by use of the @option{--image-opts} option with a raw
-blockdev using the @code{offset} and @code{size} parameters layered on
-top of any other existing blockdev. For example, if partition 1 is
-100MiB long starting at 1MiB, the old command:
-
-@code{qemu-nbd -t -P 1 -f qcow2 file.qcow2}
-
-can be rewritten as:
-
-@code{qemu-nbd -t --image-opts driver=raw,offset=1M,size=100M,file.driver=qcow2,file.backing.driver=file,file.backing.filename=file.qcow2}
-
-Alternatively, the @code{nbdkit} project provides a more powerful
-partition filter on top of its nbd plugin, which can be used to select
-an arbitrary MBR or GPT partition on top of any other full-image NBD
-export.  Using this to rewrite the above example results in:
-
-@code{qemu-nbd -t -k /tmp/sock -f qcow2 file.qcow2 &}
-@code{nbdkit -f --filter=partition nbd socket=/tmp/sock partition=1}
-
-Note that if you are exposing the export via /dev/nbd0, it is easier
-to just export the entire image and then mount only /dev/nbd0p1 than
-it is to reinvoke @command{qemu-nbd -c /dev/nbd0} limited to just a
-subset of the image.
-
 @subsection qemu-img convert -n -o (since 4.2.0)

 All options specified in @option{-o} are image creation options, so
@@ -383,3 +352,21 @@ trouble after a recent upgrade.

 The "autoload" parameter has been ignored since 2.12.0. All bitmaps
 are automatically loaded from qcow2 images.
+
+@section Related binaries
+
+@subsection qemu-nbd --partition (removed in 5.0.0)
+
+The ``qemu-nbd --partition $digit'' code (also spelled @option{-P})
+could only handle MBR partitions, and never correctly handled logical
+partitions beyond partition 5.  Exporting a partition can still be
+done by utilizing the @option{--image-opts} option with a raw blockdev
+using the @code{offset} and @code{size} parameters layered on top of
+any other existing blockdev. For example, if partition 1 is 100MiB
+long starting at 1MiB, the old command:
+
+@code{qemu-nbd -t -P 1 -f qcow2 file.qcow2}
+
+can be rewritten as:
+
+@code{qemu-nbd -t --image-opts driver=raw,offset=1M,size=100M,file.driver=qcow2,file.backing.driver=file,file.backing.filename=file.qcow2}
diff --git a/qemu-nbd.c b/qemu-nbd.c
index bc125370dd36..e6f2eb76a3f2 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -100,7 +100,6 @@ static void usage(const char *name)
 "\n"
 "Exposing part of the image:\n"
 "  -o, --offset=OFFSET       offset into the image\n"
-"  -P, --partition=NUM       only expose partition NUM\n"
 "  -B, --bitmap=NAME         expose a persistent dirty bitmap\n"
 "\n"
 "General purpose options:\n"
@@ -156,96 +155,6 @@ QEMU_COPYRIGHT "\n"
     , name);
 }

-struct partition_record
-{
-    uint8_t bootable;
-    uint8_t start_head;
-    uint32_t start_cylinder;
-    uint8_t start_sector;
-    uint8_t system;
-    uint8_t end_head;
-    uint8_t end_cylinder;
-    uint8_t end_sector;
-    uint32_t start_sector_abs;
-    uint32_t nb_sectors_abs;
-};
-
-static void read_partition(uint8_t *p, struct partition_record *r)
-{
-    r->bootable = p[0];
-    r->start_head = p[1];
-    r->start_cylinder = p[3] | ((p[2] << 2) & 0x0300);
-    r->start_sector = p[2] & 0x3f;
-    r->system = p[4];
-    r->end_head = p[5];
-    r->end_cylinder = p[7] | ((p[6] << 2) & 0x300);
-    r->end_sector = p[6] & 0x3f;
-
-    r->start_sector_abs = ldl_le_p(p + 8);
-    r->nb_sectors_abs   = ldl_le_p(p + 12);
-}
-
-static int find_partition(BlockBackend *blk, int partition,
-                          uint64_t *offset, uint64_t *size)
-{
-    struct partition_record mbr[4];
-    uint8_t data[MBR_SIZE];
-    int i;
-    int ext_partnum = 4;
-    int ret;
-
-    ret = blk_pread(blk, 0, data, sizeof(data));
-    if (ret < 0) {
-        error_report("error while reading: %s", strerror(-ret));
-        exit(EXIT_FAILURE);
-    }
-
-    if (data[510] != 0x55 || data[511] != 0xaa) {
-        return -EINVAL;
-    }
-
-    for (i = 0; i < 4; i++) {
-        read_partition(&data[446 + 16 * i], &mbr[i]);
-
-        if (!mbr[i].system || !mbr[i].nb_sectors_abs) {
-            continue;
-        }
-
-        if (mbr[i].system == 0xF || mbr[i].system == 0x5) {
-            struct partition_record ext[4];
-            uint8_t data1[MBR_SIZE];
-            int j;
-
-            ret = blk_pread(blk, mbr[i].start_sector_abs * MBR_SIZE,
-                            data1, sizeof(data1));
-            if (ret < 0) {
-                error_report("error while reading: %s", strerror(-ret));
-                exit(EXIT_FAILURE);
-            }
-
-            for (j = 0; j < 4; j++) {
-                read_partition(&data1[446 + 16 * j], &ext[j]);
-                if (!ext[j].system || !ext[j].nb_sectors_abs) {
-                    continue;
-                }
-
-                if ((ext_partnum + j + 1) == partition) {
-                    *offset = (uint64_t)ext[j].start_sector_abs << 9;
-                    *size = (uint64_t)ext[j].nb_sectors_abs << 9;
-                    return 0;
-                }
-            }
-            ext_partnum += 4;
-        } else if ((i + 1) == partition) {
-            *offset = (uint64_t)mbr[i].start_sector_abs << 9;
-            *size = (uint64_t)mbr[i].nb_sectors_abs << 9;
-            return 0;
-        }
-    }
-
-    return -ENOENT;
-}
-
 static void termsig_handler(int signum)
 {
     atomic_cmpxchg(&state, RUNNING, TERMINATE);
@@ -620,7 +529,7 @@ int main(int argc, char **argv)
     int64_t fd_size;
     QemuOpts *sn_opts = NULL;
     const char *sn_id_or_name = NULL;
-    const char *sopt = "hVb:o:p:rsnP:c:dvk:e:f:tl:x:T:D:B:L";
+    const char *sopt = "hVb:o:p:rsnc:dvk:e:f:tl:x:T:D:B:L";
     struct option lopt[] = {
         { "help", no_argument, NULL, 'h' },
         { "version", no_argument, NULL, 'V' },
@@ -629,7 +538,6 @@ int main(int argc, char **argv)
         { "socket", required_argument, NULL, 'k' },
         { "offset", required_argument, NULL, 'o' },
         { "read-only", no_argument, NULL, 'r' },
-        { "partition", required_argument, NULL, 'P' },
         { "bitmap", required_argument, NULL, 'B' },
         { "connect", required_argument, NULL, 'c' },
         { "disconnect", no_argument, NULL, 'd' },
@@ -660,7 +568,6 @@ int main(int argc, char **argv)
     int ch;
     int opt_ind = 0;
     int flags = BDRV_O_RDWR;
-    int partition = 0;
     int ret = 0;
     bool seen_cache = false;
     bool seen_discard = false;
@@ -796,15 +703,6 @@ int main(int argc, char **argv)
             readonly = true;
             flags &= ~BDRV_O_RDWR;
             break;
-        case 'P':
-            warn_report("The '-P' option is deprecated; use --image-opts with "
-                        "a raw device wrapper for subset exports instead");
-            if (qemu_strtoi(optarg, NULL, 0, &partition) < 0 ||
-                partition < 1 || partition > 8) {
-                error_report("Invalid partition '%s'", optarg);
-                exit(EXIT_FAILURE);
-            }
-            break;
         case 'B':
             bitmap = optarg;
             break;
@@ -901,7 +799,7 @@ int main(int argc, char **argv)
             error_report("List mode is incompatible with a file name");
             exit(EXIT_FAILURE);
         }
-        if (export_name || export_description || dev_offset || partition ||
+        if (export_name || export_description || dev_offset ||
             device || disconnect || fmt || sn_id_or_name || bitmap ||
             seen_aio || seen_discard || seen_cache) {
             error_report("List mode is incompatible with per-device settings");
@@ -1165,33 +1063,6 @@ int main(int argc, char **argv)
     }
     fd_size -= dev_offset;

-    if (partition) {
-        uint64_t limit;
-
-        if (dev_offset) {
-            error_report("Cannot request partition and offset together");
-            exit(EXIT_FAILURE);
-        }
-        ret = find_partition(blk, partition, &dev_offset, &limit);
-        if (ret < 0) {
-            error_report("Could not find partition %d: %s", partition,
-                         strerror(-ret));
-            exit(EXIT_FAILURE);
-        }
-        /*
-         * MBR partition limits are (32-bit << 9); this assert lets
-         * the compiler know that we can't overflow 64 bits.
-         */
-        assert(dev_offset + limit >= dev_offset);
-        if (dev_offset + limit > fd_size) {
-            error_report("Discovered partition %d at offset %" PRIu64
-                         " size %" PRIu64 ", but size exceeds file length %"
-                         PRId64, partition, dev_offset, limit, fd_size);
-            exit(EXIT_FAILURE);
-        }
-        fd_size = limit;
-    }
-
     export = nbd_export_new(bs, dev_offset, fd_size, export_name,
                             export_description, bitmap, readonly, shared > 1,
                             nbd_export_closed, writethrough, NULL,
diff --git a/qemu-nbd.texi b/qemu-nbd.texi
index 7f55657722bd..36f4188a3f27 100644
--- a/qemu-nbd.texi
+++ b/qemu-nbd.texi
@@ -55,11 +55,6 @@ Force the use of the block driver for format @var{fmt} instead of
 auto-detecting.
 @item -r, --read-only
 Export the disk as read-only.
-@item -P, --partition=@var{num}
-Deprecated: Only expose MBR partition @var{num}.  Understands physical
-partitions 1-4 and logical partition 5. New code should instead use
-@option{--image-opts} with the raw driver wrapping a subset of the
-original image.
 @item -B, --bitmap=@var{name}
 If @var{filename} has a qcow2 persistent bitmap @var{name}, expose
 that bitmap via the ``qemu:dirty-bitmap:@var{name}'' context
@@ -161,13 +156,13 @@ qemu-nbd \
   --image-opts driver=raw,offset=1M,size=1M,file.driver=file,file.filename=file.raw
 @end example

-Serve a read-only copy of just the first MBR partition of a guest
-image over a Unix socket with as many as 5 simultaneous readers, with
-a persistent process forked as a daemon:
+Serve a read-only copy of a guest image over a Unix socket with as
+many as 5 simultaneous readers, with a persistent process forked as a
+daemon:

 @example
 qemu-nbd --fork --persistent --shared=5 --socket=/path/to/sock \
-  --partition=1 --read-only --format=qcow2 file.qcow2
+  --read-only --format=qcow2 file.qcow2
 @end example

 Expose the guest-visible contents of a qcow2 file via a block device
-- 
2.24.1

Re: [PATCH] qemu-nbd: Removed deprecated --partition option

[no-reply]

Patchew URL: https://patchew.org/QEMU/20200122214328.1413664-1-eblake@redhat.com/



Hi,

This series failed the docker-quick@centos7 build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.

=== TEST SCRIPT BEGIN ===
#!/bin/bash
make docker-image-centos7 V=1 NETWORK=1
time make docker-test-quick@centos7 SHOW_ENV=1 J=14 NETWORK=1
=== TEST SCRIPT END ===

  TEST    check-qtest-aarch64: tests/qtest/boot-serial-test
  TEST    check-qtest-aarch64: tests/qtest/migration-test
**
ERROR:/tmp/qemu-test/src/tests/test-char.c:1125:char_serial_test: 'chr' should not be NULL
ERROR - Bail out! ERROR:/tmp/qemu-test/src/tests/test-char.c:1125:char_serial_test: 'chr' should not be NULL
make: *** [check-unit] Error 1
make: *** Waiting for unfinished jobs....
qemu-system-aarch64: -accel kvm: invalid accelerator kvm
qemu-system-aarch64: falling back to tcg
---
    raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', '--label', 'com.qemu.instance.uuid=bcad5894bb1b43f2b125072f6631cf33', '-u', '1003', '--security-opt', 'seccomp=unconfined', '--rm', '-e', 'TARGET_LIST=', '-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', '-e', 'SHOW_ENV=1', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', '/home/patchew2/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', '/var/tmp/patchew-tester-tmp-7pp43kkd/src/docker-src.2020-01-22-16.49.30.24833:/var/tmp/qemu:z,ro', 'qemu:centos7', '/var/tmp/qemu/run', 'test-quick']' returned non-zero exit status 2.
filter=--filter=label=com.qemu.instance.uuid=bcad5894bb1b43f2b125072f6631cf33
make[1]: *** [docker-run] Error 1
make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-7pp43kkd/src'
make: *** [docker-run-test-quick@centos7] Error 2

real    11m21.905s
user    0m8.570s


The full log is available at
http://patchew.org/logs/20200122214328.1413664-1-eblake@redhat.com/testing.docker-quick@centos7/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [PATCH] qemu-nbd: Removed deprecated --partition option

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 3553 bytes --]

On 22.01.20 22:43, Eric Blake wrote:
> The option was deprecated in 4.0.0 (commit 0ae2d546); it's now been
> long enough with no complaints to follow through with that process.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>  qemu-deprecated.texi |  49 ++++++----------
>  qemu-nbd.c           | 133 +------------------------------------------
>  qemu-nbd.texi        |  13 ++---
>  3 files changed, 24 insertions(+), 171 deletions(-)
> 
> diff --git a/qemu-deprecated.texi b/qemu-deprecated.texi
> index 8471eef9c22d..1b4c638db8e0 100644
> --- a/qemu-deprecated.texi
> +++ b/qemu-deprecated.texi
> @@ -304,37 +304,6 @@ The above, converted to the current supported format:
> 
>  @section Related binaries
> 
> -@subsection qemu-nbd --partition (since 4.0.0)
> -
> -The ``qemu-nbd --partition $digit'' code (also spelled @option{-P})
> -can only handle MBR partitions, and has never correctly handled
> -logical partitions beyond partition 5.  If you know the offset and
> -length of the partition (perhaps by using @code{sfdisk} within the
> -guest), you can achieve the effect of exporting just that subset of
> -the disk by use of the @option{--image-opts} option with a raw
> -blockdev using the @code{offset} and @code{size} parameters layered on
> -top of any other existing blockdev. For example, if partition 1 is
> -100MiB long starting at 1MiB, the old command:
> -
> -@code{qemu-nbd -t -P 1 -f qcow2 file.qcow2}
> -
> -can be rewritten as:
> -
> -@code{qemu-nbd -t --image-opts driver=raw,offset=1M,size=100M,file.driver=qcow2,file.backing.driver=file,file.backing.filename=file.qcow2}
> -
> -Alternatively, the @code{nbdkit} project provides a more powerful
> -partition filter on top of its nbd plugin, which can be used to select
> -an arbitrary MBR or GPT partition on top of any other full-image NBD
> -export.  Using this to rewrite the above example results in:
> -
> -@code{qemu-nbd -t -k /tmp/sock -f qcow2 file.qcow2 &}
> -@code{nbdkit -f --filter=partition nbd socket=/tmp/sock partition=1}
> -
> -Note that if you are exposing the export via /dev/nbd0, it is easier
> -to just export the entire image and then mount only /dev/nbd0p1 than
> -it is to reinvoke @command{qemu-nbd -c /dev/nbd0} limited to just a
> -subset of the image.
> -
>  @subsection qemu-img convert -n -o (since 4.2.0)
> 
>  All options specified in @option{-o} are image creation options, so
> @@ -383,3 +352,21 @@ trouble after a recent upgrade.
> 
>  The "autoload" parameter has been ignored since 2.12.0. All bitmaps
>  are automatically loaded from qcow2 images.
> +
> +@section Related binaries
> +
> +@subsection qemu-nbd --partition (removed in 5.0.0)
> +
> +The ``qemu-nbd --partition $digit'' code (also spelled @option{-P})
> +could only handle MBR partitions, and never correctly handled logical
> +partitions beyond partition 5.  Exporting a partition can still be
> +done by utilizing the @option{--image-opts} option with a raw blockdev
> +using the @code{offset} and @code{size} parameters layered on top of
> +any other existing blockdev. For example, if partition 1 is 100MiB
> +long starting at 1MiB, the old command:
> +
> +@code{qemu-nbd -t -P 1 -f qcow2 file.qcow2}
> +
> +can be rewritten as:
> +
> +@code{qemu-nbd -t --image-opts driver=raw,offset=1M,size=100M,file.driver=qcow2,file.backing.driver=file,file.backing.filename=file.qcow2}

I know you just moved it from above, but isn’t this wrong?  Shouldn’t it
be s/backing/file/g?

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] qemu-nbd: Removed deprecated --partition option

[Eric Blake]

On 1/23/20 6:10 AM, Max Reitz wrote:
> On 22.01.20 22:43, Eric Blake wrote:
>> The option was deprecated in 4.0.0 (commit 0ae2d546); it's now been
>> long enough with no complaints to follow through with that process.
>>
>> Signed-off-by: Eric Blake <eblake@redhat.com>
>> ---

>> -@code{qemu-nbd -t -P 1 -f qcow2 file.qcow2}
>> -
>> -can be rewritten as:
>> -
>> -@code{qemu-nbd -t --image-opts driver=raw,offset=1M,size=100M,file.driver=qcow2,file.backing.driver=file,file.backing.filename=file.qcow2}
>> -

>> +can be rewritten as:
>> +
>> +@code{qemu-nbd -t --image-opts driver=raw,offset=1M,size=100M,file.driver=qcow2,file.backing.driver=file,file.backing.filename=file.qcow2}
> 
> I know you just moved it from above, but isn’t this wrong?  Shouldn’t it
> be s/backing/file/g?

Indeed; file.file.driver=file,file.file.filename=file.qcow2 is required 
for it to work, rather than fail with
qemu-nbd: Failed to blk_new_open 
'driver=raw,offset=1m,size=100m,file.driver=qcow2,file.backing.driver=file,file.backing.filename=file4': 
A block device must be specified for "file"

I'll repost with the bug-fix as a separate commit (and it's a shame that 
we've gone nearly a year with no one noticing the typo in the original).

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH] qemu-nbd: Removed deprecated --partition option

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 1638 bytes --]

On 23.01.20 13:30, Eric Blake wrote:
> On 1/23/20 6:10 AM, Max Reitz wrote:
>> On 22.01.20 22:43, Eric Blake wrote:
>>> The option was deprecated in 4.0.0 (commit 0ae2d546); it's now been
>>> long enough with no complaints to follow through with that process.
>>>
>>> Signed-off-by: Eric Blake <eblake@redhat.com>
>>> ---
> 
>>> -@code{qemu-nbd -t -P 1 -f qcow2 file.qcow2}
>>> -
>>> -can be rewritten as:
>>> -
>>> -@code{qemu-nbd -t --image-opts
>>> driver=raw,offset=1M,size=100M,file.driver=qcow2,file.backing.driver=file,file.backing.filename=file.qcow2}
>>>
>>> -
> 
>>> +can be rewritten as:
>>> +
>>> +@code{qemu-nbd -t --image-opts
>>> driver=raw,offset=1M,size=100M,file.driver=qcow2,file.backing.driver=file,file.backing.filename=file.qcow2}
>>>
>>
>> I know you just moved it from above, but isn’t this wrong?  Shouldn’t it
>> be s/backing/file/g?
> 
> Indeed; file.file.driver=file,file.file.filename=file.qcow2 is required
> for it to work, rather than fail with
> qemu-nbd: Failed to blk_new_open
> 'driver=raw,offset=1m,size=100m,file.driver=qcow2,file.backing.driver=file,file.backing.filename=file4':
> A block device must be specified for "file"
> 
> I'll repost with the bug-fix as a separate commit (and it's a shame that
> we've gone nearly a year with no one noticing the typo in the original).

It was actually meant as a service to our users!  So they can’t just
blindly copy-paste the example but have to think it through.  And the
fact that noone has complained shows that our users indeed didn’t let
themselves be caught off-guard!  /s O:-)

Thanks!

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]