diff mbox series

[1/2] xl: implement documented --force option for block-detach

Message ID 20200903100537.1337-2-paul@xen.org
State Superseded
Headers show
Series fix 'xl block-detach' | expand

Commit Message

Paul Durrant Sept. 3, 2020, 10:05 a.m. UTC
From: Paul Durrant <pdurrant@amazon.com>

The manpage for 'xl' documents an option to force a block device to be
released even if the domain to which it is attached does not co-operate.
This option, however, is not implemented. This patch implements the option.

NOTE: The documentation is also adjusted since the normal positioning of
      options is before compulsory parameters. It is also noted that use of
      the --force option may lead to a guest crash.

Signed-off-by: Paul Durrant <pdurrant@amazon.com>
---
Cc: Ian Jackson <iwj@xenproject.org>
Cc: Wei Liu <wl@xen.org>
Cc: Anthony PERARD <anthony.perard@citrix.com>
---
 docs/man/xl.1.pod.in   |  4 ++--
 tools/xl/xl_block.c    | 21 ++++++++++++++++-----
 tools/xl/xl_cmdtable.c |  3 ++-
 3 files changed, 20 insertions(+), 8 deletions(-)

Comments

Wei Liu Sept. 8, 2020, 2:02 p.m. UTC | #1
On Thu, Sep 03, 2020 at 11:05:36AM +0100, Paul Durrant wrote:
> From: Paul Durrant <pdurrant@amazon.com>
> 
> The manpage for 'xl' documents an option to force a block device to be
> released even if the domain to which it is attached does not co-operate.
> This option, however, is not implemented. This patch implements the option.
> 
> NOTE: The documentation is also adjusted since the normal positioning of
>       options is before compulsory parameters. It is also noted that use of
>       the --force option may lead to a guest crash.
> 
> Signed-off-by: Paul Durrant <pdurrant@amazon.com>
> ---
> Cc: Ian Jackson <iwj@xenproject.org>
> Cc: Wei Liu <wl@xen.org>
> Cc: Anthony PERARD <anthony.perard@citrix.com>
> ---
>  docs/man/xl.1.pod.in   |  4 ++--
>  tools/xl/xl_block.c    | 21 ++++++++++++++++-----
>  tools/xl/xl_cmdtable.c |  3 ++-
>  3 files changed, 20 insertions(+), 8 deletions(-)
> 
> diff --git a/docs/man/xl.1.pod.in b/docs/man/xl.1.pod.in
> index 52a47a6fbd..5f7d3a7134 100644
> --- a/docs/man/xl.1.pod.in
> +++ b/docs/man/xl.1.pod.in
> @@ -1389,7 +1389,7 @@ Note that only PV block devices are supported by block-attach.
>  Requests to attach emulated devices (eg, vdev=hdc) will result in only
>  the PV view being available to the guest.
>  
> -=item B<block-detach> I<domain-id> I<devid> [I<OPTIONS>]
> +=item B<block-detach> [I<OPTIONS>] I<domain-id> I<devid>
>  
>  Detach a domain's virtual block device. I<devid> may be the symbolic
>  name or the numeric device id given to the device by domain 0.  You
> @@ -1406,7 +1406,7 @@ B<OPTIONS>
>  =item B<--force>
>  
>  If this parameter is specified the device will be forcefully detached, which
> -may cause IO errors in the domain.
> +may cause IO errors in the domain and possibly a guest crash

Missing "." at the end. This can be fixed while committing.

Acked-by: Wei Liu <wl@xen.org>
diff mbox series

Patch

diff --git a/docs/man/xl.1.pod.in b/docs/man/xl.1.pod.in
index 52a47a6fbd..5f7d3a7134 100644
--- a/docs/man/xl.1.pod.in
+++ b/docs/man/xl.1.pod.in
@@ -1389,7 +1389,7 @@  Note that only PV block devices are supported by block-attach.
 Requests to attach emulated devices (eg, vdev=hdc) will result in only
 the PV view being available to the guest.
 
-=item B<block-detach> I<domain-id> I<devid> [I<OPTIONS>]
+=item B<block-detach> [I<OPTIONS>] I<domain-id> I<devid>
 
 Detach a domain's virtual block device. I<devid> may be the symbolic
 name or the numeric device id given to the device by domain 0.  You
@@ -1406,7 +1406,7 @@  B<OPTIONS>
 =item B<--force>
 
 If this parameter is specified the device will be forcefully detached, which
-may cause IO errors in the domain.
+may cause IO errors in the domain and possibly a guest crash
 
 =back
 
diff --git a/tools/xl/xl_block.c b/tools/xl/xl_block.c
index acaf9b96b8..05696643bf 100644
--- a/tools/xl/xl_block.c
+++ b/tools/xl/xl_block.c
@@ -96,12 +96,21 @@  int main_blocklist(int argc, char **argv)
 
 int main_blockdetach(int argc, char **argv)
 {
+    static struct option opts[] = {
+        {"force", 0, 0, 'f'},
+        COMMON_LONG_OPTS
+    };
     uint32_t domid;
     int opt, rc = 0;
     libxl_device_disk disk;
-
-    SWITCH_FOREACH_OPT(opt, "", NULL, "block-detach", 2) {
-        /* No options */
+    bool force = false;
+
+    SWITCH_FOREACH_OPT(opt, "f", opts, "block-detach", 2) {
+    case 'f':
+        force = true;
+        break;
+    default:
+        break;
     }
 
     domid = find_domain(argv[optind]);
@@ -110,9 +119,11 @@  int main_blockdetach(int argc, char **argv)
         fprintf(stderr, "Error: Device %s not connected.\n", argv[optind+1]);
         return 1;
     }
-    rc = libxl_device_disk_remove(ctx, domid, &disk, 0);
+    rc = !force ? libxl_device_disk_remove(ctx, domid, &disk, 0) :
+        libxl_device_disk_destroy(ctx, domid, &disk, 0);
     if (rc) {
-        fprintf(stderr, "libxl_device_disk_remove failed.\n");
+        fprintf(stderr, "libxl_device_disk_%s failed.\n",
+                !force ? "remove" : "destroy");
         return 1;
     }
     libxl_device_disk_dispose(&disk);
diff --git a/tools/xl/xl_cmdtable.c b/tools/xl/xl_cmdtable.c
index 2b8e1b321a..7da6c1b927 100644
--- a/tools/xl/xl_cmdtable.c
+++ b/tools/xl/xl_cmdtable.c
@@ -368,7 +368,8 @@  struct cmd_spec cmd_table[] = {
     { "block-detach",
       &main_blockdetach, 0, 1,
       "Destroy a domain's virtual block device",
-      "<Domain> <DevId>",
+      "[option] <Domain> <DevId>",
+      "-f, --force        do not wait for the domain to release the device"
     },
     { "vtpm-attach",
       &main_vtpmattach, 1, 1,