[PATCH v2 0/2] finish qemu-nbd --partition deprecation

[PATCH v2 0/2] finish qemu-nbd --partition deprecation

[Eric Blake]

Based-on: <20200116141511.16849-1-peter.maydell@linaro.org>
(0/3 convert qemu-nbd, qemu-block-drivers to rST)

In v2:
- rebased on top of rST doc changes
- patch 1 added

Eric Blake (2):
  docs: Fix typo in qemu-nbd -P replacement
  qemu-nbd: Removed deprecated --partition option

 docs/interop/qemu-nbd.rst |  15 ++---
 qemu-deprecated.texi      |  49 ++++++--------
 qemu-nbd.c                | 133 +-------------------------------------
 3 files changed, 24 insertions(+), 173 deletions(-)

-- 
2.24.1

[PATCH v2 1/2] docs: Fix typo in qemu-nbd -P replacement

[Eric Blake]

The suggested replacement for the deprecated 'qemu-nbd -P' referw to
'file.backing.opt' instead of 'file.file.opt'; using the example
verbatim results in:

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"

Correct this text, prior to actually finishing the deprecation process.

Fixes: 0ae2d54645eb
Reported-by: Max Reitz <mreitz@redhat.com>
Signed-off-by: Eric Blake <eblake@redhat.com>
---
 qemu-deprecated.texi | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/qemu-deprecated.texi b/qemu-deprecated.texi
index 0968d37c745a..358eb6deebdc 100644
--- a/qemu-deprecated.texi
+++ b/qemu-deprecated.texi
@@ -335,7 +335,7 @@ top of any other existing blockdev. For example, if partition 1 is

 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}
+@code{qemu-nbd -t --image-opts driver=raw,offset=1M,size=100M,file.driver=qcow2,file.file.driver=file,file.file.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
-- 
2.24.1

[PATCH v2 2/2] 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>
---
 docs/interop/qemu-nbd.rst |  15 ++---
 qemu-deprecated.texi      |  49 ++++++--------
 qemu-nbd.c                | 133 +-------------------------------------
 3 files changed, 24 insertions(+), 173 deletions(-)

diff --git a/docs/interop/qemu-nbd.rst b/docs/interop/qemu-nbd.rst
index 873bb9e17d56..2e20f84cf025 100644
--- a/docs/interop/qemu-nbd.rst
+++ b/docs/interop/qemu-nbd.rst
@@ -72,13 +72,6 @@ driver options if ``--image-opts`` is specified.

   Export the disk as read-only.

-.. option:: -P, --partition=NUM
-
-  Deprecated: Only expose MBR partition *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.
-
 .. option:: -B, --bitmap=NAME

   If *filename* has a qcow2 persistent bitmap *NAME*, expose
@@ -224,14 +217,14 @@ a 1 megabyte subset of a raw file, using the export name 'subset':
     -t -x subset -p 10810 \
     --image-opts driver=raw,offset=1M,size=1M,file.driver=file,file.filename=file.raw

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

 ::

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

 Expose the guest-visible contents of a qcow2 file via a block device
 /dev/nbd0 (and possibly creating /dev/nbd0p1 and friends for
diff --git a/qemu-deprecated.texi b/qemu-deprecated.texi
index 358eb6deebdc..f152e8816164 100644
--- a/qemu-deprecated.texi
+++ b/qemu-deprecated.texi
@@ -319,37 +319,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.file.driver=file,file.file.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
@@ -406,3 +375,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.file.driver=file,file.file.filename=file.qcow2}
diff --git a/qemu-nbd.c b/qemu-nbd.c
index 108a51f7eb01..a04930770ff7 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);
@@ -617,7 +526,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' },
@@ -626,7 +535,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' },
@@ -657,7 +565,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;
@@ -793,15 +700,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;
@@ -898,7 +796,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");
@@ -1162,33 +1060,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,
-- 
2.24.1

Re: [PATCH v2 0/2] finish qemu-nbd --partition deprecation

[Eric Blake]

ping

On 1/23/20 10:46 AM, Eric Blake wrote:
> Based-on: <20200116141511.16849-1-peter.maydell@linaro.org>
> (0/3 convert qemu-nbd, qemu-block-drivers to rST)
> 
> In v2:
> - rebased on top of rST doc changes
> - patch 1 added
> 
> Eric Blake (2):
>    docs: Fix typo in qemu-nbd -P replacement
>    qemu-nbd: Removed deprecated --partition option
> 
>   docs/interop/qemu-nbd.rst |  15 ++---
>   qemu-deprecated.texi      |  49 ++++++--------
>   qemu-nbd.c                | 133 +-------------------------------------
>   3 files changed, 24 insertions(+), 173 deletions(-)
> 

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

Re: [PATCH v2 1/2] docs: Fix typo in qemu-nbd -P replacement

[Ján Tomko]

[-- Attachment #1: Type: text/plain, Size: 768 bytes --]

On Thu, Jan 23, 2020 at 10:46:49AM -0600, Eric Blake wrote:
>The suggested replacement for the deprecated 'qemu-nbd -P' referw to

s/referw/refer/

>'file.backing.opt' instead of 'file.file.opt'; using the example
>verbatim results in:
>
>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"
>
>Correct this text, prior to actually finishing the deprecation process.
>
>Fixes: 0ae2d54645eb
>Reported-by: Max Reitz <mreitz@redhat.com>
>Signed-off-by: Eric Blake <eblake@redhat.com>
>---
> qemu-deprecated.texi | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>

Reviewed-by: Ján Tomko <jtomko@redhat.com>

Jano

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

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

[Ján Tomko]

[-- Attachment #1: Type: text/plain, Size: 535 bytes --]

On Thu, Jan 23, 2020 at 10:46:50AM -0600, 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>
>---
> docs/interop/qemu-nbd.rst |  15 ++---
> qemu-deprecated.texi      |  49 ++++++--------
> qemu-nbd.c                | 133 +-------------------------------------
> 3 files changed, 24 insertions(+), 173 deletions(-)
>

Reviewed-by: Ján Tomko <jtomko@redhat.com>

Jano

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

Re: [PATCH v2 0/2] finish qemu-nbd --partition deprecation

[Max Reitz]

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

On 31.01.20 18:11, Eric Blake wrote:
> ping

Do you want further review or is Ján’s sufficient for you?

Also, I wonder whether it would make a good GSoC/Outreachy/... project
to add partition reading support to the raw block driver, or whether
that’s a bad idea. O:-)

Max


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

Re: [PATCH v2 0/2] finish qemu-nbd --partition deprecation

[Eric Blake]

On 2/19/20 4:53 AM, Max Reitz wrote:
> On 31.01.20 18:11, Eric Blake wrote:
>> ping
> 
> Do you want further review or is Ján’s sufficient for you?

Commit 0bc16997 has already landed, so no further review will show in 
git history. But you're always welcome to raise issues that might result 
in follow-up patches.

> 
> Also, I wonder whether it would make a good GSoC/Outreachy/... project
> to add partition reading support to the raw block driver, or whether
> that’s a bad idea. O:-)

Personally, I think that since nbdkit already provides a good partition 
filter [1], any other implementation of a partition filter is duplicated 
effort.  The only reason to teach qemu how to access an arbitrary 
partition is if it makes serving that arbitrary partition to a guest 
more efficient than what is currently possible by pointing qemu at an 
NBD server run by nbdkit with its partition filter.  But in general, you 
are more likely to want qemu to serve an entire disk image file to the 
guest (and let the guest interpret partitions embedded within that file) 
than to have qemu serve a single partition to a guest (and have the 
guest treat that partition as a bare disk), so I don't see it as 
something that would be frequently used or needed in qemu.

[1] http://libguestfs.org/nbdkit-partition-filter.1.html

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