mbox series

[v3,00/13] RFC: [for 5.0]: HMP monitor handlers cleanups

Message ID 20200127103647.17761-1-mlevitsk@redhat.com
Headers show
Series RFC: [for 5.0]: HMP monitor handlers cleanups | expand

Message

Maxim Levitsky Jan. 27, 2020, 10:36 a.m. UTC 
This patch series is bunch of cleanups
to the hmp monitor code.

This series only touched blockdev related hmp handlers.

No functional changes expected other that
light error message changes by the last patch.

This was inspired by this bugzilla:
https://bugzilla.redhat.com/show_bug.cgi?id=1719169

Basically some users still parse hmp error messages,
and they would like to have them prefixed with 'Error:'

In commit 66363e9a43f649360a3f74d2805c9f864da027eb we added
the hmp_handle_error which does exactl that but some hmp handlers
don't use it.

In this patch series, I moved all the block related hmp handlers
into blockdev-hmp-cmds.c, and then made them use this function
to report the errors.

I hope I didn't change too much code, I just felt that if
I touch this code, I can also make it easier to find these
handlers, that were scattered over 3 different files.

Changes from V1:
   * move the handlers to block/monitor/block-hmp-cmds.c
   * tiny cleanup for the commit messages

Changes from V2:
   * Moved all the function prototypes to new header (blockdev-hmp-cmds.h)
   * Set the license of blockdev-hmp-cmds.c to GPLv2+
   * Moved hmp_snapshot_* functions to blockdev-hmp-cmds.c
   * Moved hmp_drive_add_node to blockdev-hmp-cmds.c
     (this change needed some new exports, thus in separate new patch)
   * Moved hmp_qemu_io and hmp_eject to blockdev-hmp-cmds.c
   * Added 'error:' prefix to vreport, and updated the iotests
     This is invasive change, but really feels like the right one
   * Added minor refactoring patch that drops an unused #include

Best regards,
	Maxim Levitsky

Maxim Levitsky (13):
  usb/dev-storage: remove unused include
  monitor/hmp: uninline add_init_drive
  monitor/hmp: rename device-hotplug.c to block/monitor/block-hmp-cmds.c
  monitor/hmp: move hmp_drive_del and hmp_commit to block-hmp-cmds.c
  monitor/hmp: move hmp_drive_mirror and hmp_drive_backup to
    block-hmp-cmds.c
  monitor/hmp: move hmp_block_job* to block-hmp-cmds.c
  monitor/hmp: move hmp_snapshot_* to block-hmp-cmds.c
  monitor/hmp: move hmp_nbd_server* to block-hmp-cmds.c
  monitor/hmp: move remaining hmp_block* functions to block-hmp-cmds.c
  monitor/hmp: move hmp_info_block* to block-hmp-cmds.c
  monitor: Move hmp_drive_add_node to block-hmp-cmds.c
  add 'error' prefix to vreport
  monitor/hmp: Prefer to use hmp_handle_error for error reporting in
    block hmp commands

 MAINTAINERS                        |   1 +
 Makefile.objs                      |   2 +-
 block/Makefile.objs                |   1 +
 block/monitor/Makefile.objs        |   1 +
 block/monitor/block-hmp-cmds.c     | 980 +++++++++++++++++++++++++++++
 blockdev.c                         | 137 +---
 device-hotplug.c                   |  91 ---
 hw/usb/dev-storage.c               |   1 -
 include/block/block-hmp-commands.h |  42 ++
 include/block/block_int.h          |   5 +-
 include/monitor/hmp.h              |  24 -
 include/sysemu/blockdev.h          |   4 -
 include/sysemu/sysemu.h            |   3 -
 monitor/hmp-cmds.c                 | 771 +----------------------
 monitor/misc.c                     |   1 +
 tests/qemu-iotests/020.out         |   2 +-
 tests/qemu-iotests/026.out         | 260 ++++----
 tests/qemu-iotests/036.out         |  16 +-
 tests/qemu-iotests/043.out         |   6 +-
 tests/qemu-iotests/049.out         |  30 +-
 tests/qemu-iotests/051.pc.out      | 150 ++---
 tests/qemu-iotests/054.out         |   4 +-
 tests/qemu-iotests/060.out         |  20 +-
 tests/qemu-iotests/061.out         |  26 +-
 tests/qemu-iotests/069.out         |   2 +-
 tests/qemu-iotests/071.out         |   4 +-
 tests/qemu-iotests/074.out         |   4 +-
 tests/qemu-iotests/079.out         |   2 +-
 tests/qemu-iotests/080.out         |  72 +--
 tests/qemu-iotests/081.out         |   2 +-
 tests/qemu-iotests/082.out         |  38 +-
 tests/qemu-iotests/083.out         |  68 +-
 tests/qemu-iotests/098.out         |   8 +-
 tests/qemu-iotests/103.out         |  14 +-
 tests/qemu-iotests/106.out         |   4 +-
 tests/qemu-iotests/111.out         |   2 +-
 tests/qemu-iotests/112.out         |  24 +-
 tests/qemu-iotests/113.out         |   6 +-
 tests/qemu-iotests/114.out         |   2 +-
 tests/qemu-iotests/122.out         |   4 +-
 tests/qemu-iotests/133.out         |  30 +-
 tests/qemu-iotests/137.out         |  28 +-
 tests/qemu-iotests/140.out         |   2 +-
 tests/qemu-iotests/142.out         |  38 +-
 tests/qemu-iotests/143.out         |   2 +-
 tests/qemu-iotests/153.out         | 118 ++--
 tests/qemu-iotests/162.out         |  10 +-
 tests/qemu-iotests/172.out         |  16 +-
 tests/qemu-iotests/178.out.qcow2   |  30 +-
 tests/qemu-iotests/178.out.raw     |  26 +-
 tests/qemu-iotests/182.out         |   2 +-
 tests/qemu-iotests/187.out         |   6 +-
 tests/qemu-iotests/188.out         |   2 +-
 tests/qemu-iotests/197.out         |   2 +-
 tests/qemu-iotests/205             |   2 +-
 tests/qemu-iotests/215.out         |   2 +-
 tests/qemu-iotests/217.out         |   2 +-
 tests/qemu-iotests/226.out         |  12 +-
 tests/qemu-iotests/232.out         |  12 +-
 tests/qemu-iotests/233.out         |  24 +-
 tests/qemu-iotests/242.out         |   2 +-
 tests/qemu-iotests/244.out         |  14 +-
 tests/qemu-iotests/249.out         |   6 +-
 tests/qemu-iotests/261.out         |  24 +-
 tests/qemu-iotests/267.out         |  16 +-
 tests/qemu-iotests/common.filter   |   2 +-
 util/qemu-error.c                  |   1 +
 67 files changed, 1640 insertions(+), 1625 deletions(-)
 create mode 100644 block/monitor/Makefile.objs
 create mode 100644 block/monitor/block-hmp-cmds.c
 delete mode 100644 device-hotplug.c
 create mode 100644 include/block/block-hmp-commands.h

Comments

no-reply@patchew.org Jan. 27, 2020, 10:55 a.m. UTC | #1 
Patchew URL: https://patchew.org/QEMU/20200127103647.17761-1-mlevitsk@redhat.com/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: 20200127103647.17761-1-mlevitsk@redhat.com
Subject: [PATCH v3 00/13] RFC: [for 5.0]: HMP monitor handlers cleanups

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 - [tag update]      patchew/1579779525-20065-1-git-send-email-imammedo@redhat.com -> patchew/1579779525-20065-1-git-send-email-imammedo@redhat.com
 - [tag update]      patchew/20200124100159.736209-1-stefanha@redhat.com -> patchew/20200124100159.736209-1-stefanha@redhat.com
 - [tag update]      patchew/20200124162606.8787-1-peter.maydell@linaro.org -> patchew/20200124162606.8787-1-peter.maydell@linaro.org
 - [tag update]      patchew/20200124172954.28481-1-peter.maydell@linaro.org -> patchew/20200124172954.28481-1-peter.maydell@linaro.org
 * [new tag]         patchew/20200127103647.17761-1-mlevitsk@redhat.com -> patchew/20200127103647.17761-1-mlevitsk@redhat.com
Switched to a new branch 'test'
9d7e4e6 monitor/hmp: Prefer to use hmp_handle_error for error reporting in block hmp commands
a549971 add 'error' prefix to vreport
7c7da3a monitor: Move hmp_drive_add_node to block-hmp-cmds.c
a2a0265 monitor/hmp: move hmp_info_block* to block-hmp-cmds.c
d7f13de monitor/hmp: move remaining hmp_block* functions to block-hmp-cmds.c
c067499 monitor/hmp: move hmp_nbd_server* to block-hmp-cmds.c
f5fab94 monitor/hmp: move hmp_snapshot_* to block-hmp-cmds.c
4cb26f9 monitor/hmp: move hmp_block_job* to block-hmp-cmds.c
97953d5 monitor/hmp: move hmp_drive_mirror and hmp_drive_backup to block-hmp-cmds.c
00dc3e8 monitor/hmp: move hmp_drive_del and hmp_commit to block-hmp-cmds.c
a4aa184 monitor/hmp: rename device-hotplug.c to block/monitor/block-hmp-cmds.c
d76374a monitor/hmp: uninline add_init_drive
13decc9 usb/dev-storage: remove unused include

=== OUTPUT BEGIN ===
1/13 Checking commit 13decc9a539d (usb/dev-storage: remove unused include)
2/13 Checking commit d76374a8829d (monitor/hmp: uninline add_init_drive)
3/13 Checking commit a4aa1842d39b (monitor/hmp: rename device-hotplug.c to block/monitor/block-hmp-cmds.c)
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#58: 
new file mode 100644

total: 0 errors, 1 warnings, 83 lines checked

Patch 3/13 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
4/13 Checking commit 00dc3e8c0cc5 (monitor/hmp: move hmp_drive_del and hmp_commit to block-hmp-cmds.c)
WARNING: Block comments use a leading /* on a separate line
#81: FILE: block/monitor/block-hmp-cmds.c:119:
+    /* If this BlockBackend has a device attached to it, its refcount will be

total: 0 errors, 1 warnings, 234 lines checked

Patch 4/13 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
5/13 Checking commit 97953d583c25 (monitor/hmp: move hmp_drive_mirror and hmp_drive_backup to block-hmp-cmds.c)
6/13 Checking commit 4cb26f9c9af1 (monitor/hmp: move hmp_block_job* to block-hmp-cmds.c)
7/13 Checking commit f5fab9454aca (monitor/hmp: move hmp_snapshot_* to block-hmp-cmds.c)
WARNING: Block comments use a leading /* on a separate line
#29: FILE: block/monitor/block-hmp-cmds.c:294:
+        /* In the future, if 'snapshot-file' is not specified, the snapshot

WARNING: Block comments use * on subsequent lines
#30: FILE: block/monitor/block-hmp-cmds.c:295:
+        /* In the future, if 'snapshot-file' is not specified, the snapshot
+           will be taken internally. Today it's actually required. */

WARNING: Block comments use a trailing */ on a separate line
#30: FILE: block/monitor/block-hmp-cmds.c:295:
+           will be taken internally. Today it's actually required. */

total: 0 errors, 3 warnings, 120 lines checked

Patch 7/13 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
8/13 Checking commit c06749935535 (monitor/hmp: move hmp_nbd_server* to block-hmp-cmds.c)
WARNING: Block comments use a leading /* on a separate line
#60: FILE: block/monitor/block-hmp-cmds.c:363:
+    /* Then try adding all block devices.  If one fails, close all and

total: 0 errors, 1 warnings, 217 lines checked

Patch 8/13 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
9/13 Checking commit d7f13ded16b4 (monitor/hmp: move remaining hmp_block* functions to block-hmp-cmds.c)
WARNING: Block comments use a leading /* on a separate line
#75: FILE: block/monitor/block-hmp-cmds.c:468:
+    /* qmp_block_set_io_throttle has separate parameters for the

WARNING: Block comments use a trailing */ on a separate line
#77: FILE: block/monitor/block-hmp-cmds.c:470:
+     * version has only one, so we must decide which one to pass. */

ERROR: "foo* bar" should be "foo *bar"
#105: FILE: block/monitor/block-hmp-cmds.c:498:
+    const char* device = qdict_get_str(qdict, "device");

ERROR: "foo* bar" should be "foo *bar"
#106: FILE: block/monitor/block-hmp-cmds.c:499:
+    const char* command = qdict_get_str(qdict, "command");

total: 2 errors, 2 warnings, 359 lines checked

Patch 9/13 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

10/13 Checking commit a2a0265c0910 (monitor/hmp: move hmp_info_block* to block-hmp-cmds.c)
WARNING: line over 80 characters
#67: FILE: block/monitor/block-hmp-cmds.c:593:
+        if (info->has_io_status && info->io_status != BLOCK_DEVICE_IO_STATUS_OK) {

WARNING: Block comments use a leading /* on a separate line
#381: FILE: block/monitor/block-hmp-cmds.c:907:
+            /* The ID is not guaranteed to be the same on all images, so

total: 0 errors, 2 warnings, 845 lines checked

Patch 10/13 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
11/13 Checking commit 7c7da3a311a3 (monitor: Move hmp_drive_add_node to block-hmp-cmds.c)
12/13 Checking commit a549971b349f (add 'error' prefix to vreport)
ERROR: trailing whitespace
#51: FILE: tests/qemu-iotests/026.out:6:
+Event: l1_update; errno: 5; imm: off; once: on; write $

ERROR: trailing whitespace
#62: FILE: tests/qemu-iotests/026.out:16:
+Event: l1_update; errno: 5; imm: off; once: off; write $

ERROR: trailing whitespace
#79: FILE: tests/qemu-iotests/026.out:30:
+Event: l1_update; errno: 28; imm: off; once: on; write $

ERROR: trailing whitespace
#90: FILE: tests/qemu-iotests/026.out:40:
+Event: l1_update; errno: 28; imm: off; once: off; write $

ERROR: trailing whitespace
#107: FILE: tests/qemu-iotests/026.out:54:
+Event: l2_load; errno: 5; imm: off; once: on; write $

ERROR: trailing whitespace
#116: FILE: tests/qemu-iotests/026.out:70:
+Event: l2_load; errno: 5; imm: off; once: off; write $

ERROR: trailing whitespace
#125: FILE: tests/qemu-iotests/026.out:86:
+Event: l2_load; errno: 28; imm: off; once: on; write $

ERROR: trailing whitespace
#134: FILE: tests/qemu-iotests/026.out:102:
+Event: l2_load; errno: 28; imm: off; once: off; write $

ERROR: trailing whitespace
#143: FILE: tests/qemu-iotests/026.out:118:
+Event: l2_update; errno: 5; imm: off; once: on; write $

ERROR: trailing whitespace
#154: FILE: tests/qemu-iotests/026.out:128:
+Event: l2_update; errno: 5; imm: off; once: off; write $

ERROR: trailing whitespace
#171: FILE: tests/qemu-iotests/026.out:142:
+Event: l2_update; errno: 28; imm: off; once: on; write $

ERROR: trailing whitespace
#182: FILE: tests/qemu-iotests/026.out:152:
+Event: l2_update; errno: 28; imm: off; once: off; write $

ERROR: trailing whitespace
#199: FILE: tests/qemu-iotests/026.out:166:
+Event: l2_alloc_write; errno: 5; imm: off; once: on; write $

ERROR: trailing whitespace
#210: FILE: tests/qemu-iotests/026.out:176:
+Event: l2_alloc_write; errno: 5; imm: off; once: off; write $

ERROR: trailing whitespace
#227: FILE: tests/qemu-iotests/026.out:190:
+Event: l2_alloc_write; errno: 28; imm: off; once: on; write $

ERROR: trailing whitespace
#238: FILE: tests/qemu-iotests/026.out:200:
+Event: l2_alloc_write; errno: 28; imm: off; once: off; write $

ERROR: trailing whitespace
#255: FILE: tests/qemu-iotests/026.out:214:
+Event: write_aio; errno: 5; imm: off; once: on; write $

ERROR: trailing whitespace
#266: FILE: tests/qemu-iotests/026.out:224:
+Event: write_aio; errno: 5; imm: off; once: off; write $

ERROR: trailing whitespace
#283: FILE: tests/qemu-iotests/026.out:238:
+Event: write_aio; errno: 28; imm: off; once: on; write $

ERROR: trailing whitespace
#294: FILE: tests/qemu-iotests/026.out:248:
+Event: write_aio; errno: 28; imm: off; once: off; write $

ERROR: trailing whitespace
#311: FILE: tests/qemu-iotests/026.out:262:
+Event: refblock_load; errno: 5; imm: off; once: on; write $

ERROR: trailing whitespace
#322: FILE: tests/qemu-iotests/026.out:272:
+Event: refblock_load; errno: 5; imm: off; once: off; write $

ERROR: trailing whitespace
#339: FILE: tests/qemu-iotests/026.out:286:
+Event: refblock_load; errno: 28; imm: off; once: on; write $

ERROR: trailing whitespace
#350: FILE: tests/qemu-iotests/026.out:296:
+Event: refblock_load; errno: 28; imm: off; once: off; write $

ERROR: trailing whitespace
#367: FILE: tests/qemu-iotests/026.out:310:
+Event: refblock_update_part; errno: 5; imm: off; once: on; write $

ERROR: trailing whitespace
#378: FILE: tests/qemu-iotests/026.out:320:
+Event: refblock_update_part; errno: 5; imm: off; once: off; write $

ERROR: trailing whitespace
#395: FILE: tests/qemu-iotests/026.out:334:
+Event: refblock_update_part; errno: 28; imm: off; once: on; write $

ERROR: trailing whitespace
#406: FILE: tests/qemu-iotests/026.out:344:
+Event: refblock_update_part; errno: 28; imm: off; once: off; write $

ERROR: trailing whitespace
#423: FILE: tests/qemu-iotests/026.out:358:
+Event: refblock_alloc; errno: 5; imm: off; once: on; write $

ERROR: trailing whitespace
#434: FILE: tests/qemu-iotests/026.out:368:
+Event: refblock_alloc; errno: 5; imm: off; once: off; write $

ERROR: trailing whitespace
#451: FILE: tests/qemu-iotests/026.out:382:
+Event: refblock_alloc; errno: 28; imm: off; once: on; write $

ERROR: trailing whitespace
#462: FILE: tests/qemu-iotests/026.out:392:
+Event: refblock_alloc; errno: 28; imm: off; once: off; write $

ERROR: trailing whitespace
#479: FILE: tests/qemu-iotests/026.out:406:
+Event: cluster_alloc; errno: 5; imm: off; once: on; write $

ERROR: trailing whitespace
#488: FILE: tests/qemu-iotests/026.out:416:
+Event: cluster_alloc; errno: 5; imm: off; once: off; write $

ERROR: trailing whitespace
#497: FILE: tests/qemu-iotests/026.out:426:
+Event: cluster_alloc; errno: 28; imm: off; once: on; write $

ERROR: trailing whitespace
#506: FILE: tests/qemu-iotests/026.out:436:
+Event: cluster_alloc; errno: 28; imm: off; once: off; write $

ERROR: trailing whitespace
#515: FILE: tests/qemu-iotests/026.out:449:
+Event: refblock_alloc_hookup; errno: 28; imm: off; once: on; write $

ERROR: trailing whitespace
#526: FILE: tests/qemu-iotests/026.out:459:
+Event: refblock_alloc_hookup; errno: 28; imm: off; once: off; write $

ERROR: trailing whitespace
#543: FILE: tests/qemu-iotests/026.out:473:
+Event: refblock_alloc_write; errno: 28; imm: off; once: on; write $

ERROR: trailing whitespace
#554: FILE: tests/qemu-iotests/026.out:483:
+Event: refblock_alloc_write; errno: 28; imm: off; once: off; write $

ERROR: trailing whitespace
#571: FILE: tests/qemu-iotests/026.out:497:
+Event: refblock_alloc_write_blocks; errno: 28; imm: off; once: on; write $

ERROR: trailing whitespace
#582: FILE: tests/qemu-iotests/026.out:507:
+Event: refblock_alloc_write_blocks; errno: 28; imm: off; once: off; write $

ERROR: trailing whitespace
#599: FILE: tests/qemu-iotests/026.out:521:
+Event: refblock_alloc_write_table; errno: 28; imm: off; once: on; write $

ERROR: trailing whitespace
#610: FILE: tests/qemu-iotests/026.out:531:
+Event: refblock_alloc_write_table; errno: 28; imm: off; once: off; write $

ERROR: trailing whitespace
#627: FILE: tests/qemu-iotests/026.out:545:
+Event: refblock_alloc_switch_table; errno: 28; imm: off; once: on; write $

ERROR: trailing whitespace
#638: FILE: tests/qemu-iotests/026.out:555:
+Event: refblock_alloc_switch_table; errno: 28; imm: off; once: off; write $

ERROR: trailing whitespace
#781: FILE: tests/qemu-iotests/049.out:107:
+qemu-img: error: Invalid image size specified! You may use k, M, G, T, P or E suffixes for $

ERROR: trailing whitespace
#793: FILE: tests/qemu-iotests/049.out:116:
+qemu-img: error: Invalid image size specified! You may use k, M, G, T, P or E suffixes for $

total: 48 errors, 0 warnings, 3053 lines checked

Patch 12/13 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

13/13 Checking commit 9d7e4e6ebe5d (monitor/hmp: Prefer to use hmp_handle_error for error reporting in block hmp commands)
=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20200127103647.17761-1-mlevitsk@redhat.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com
Maxim Levitsky Jan. 27, 2020, 10:59 a.m. UTC | #2 
On Mon, 2020-01-27 at 02:55 -0800, no-reply@patchew.org wrote:
> Patchew URL: https://patchew.org/QEMU/20200127103647.17761-1-mlevitsk@redhat.com/
> 
> 
> 
> Hi,
> 
> This series seems to have some coding style problems. See output below for
> more information:

All of these errors are either from code that I moved, and I wanted to
move it as is and from update of iotests, which shows that some of our
error messages end with space, which is stripped by iotest filter,
but once you update the iotest output, it appears again. I'll fix that
in later version of that WIP patch.

Best regards,
	Maxim Levitsky

> 
> Type: series
> Message-id: 20200127103647.17761-1-mlevitsk@redhat.com
> Subject: [PATCH v3 00/13] RFC: [for 5.0]: HMP monitor handlers cleanups
> 
> === TEST SCRIPT BEGIN ===
> #!/bin/bash
> git rev-parse base > /dev/null || exit 0
> git config --local diff.renamelimit 0
> git config --local diff.renames True
> git config --local diff.algorithm histogram
> ./scripts/checkpatch.pl --mailback base..
> === TEST SCRIPT END ===
> 
> Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
> From https://github.com/patchew-project/qemu
>  - [tag update]      patchew/1579779525-20065-1-git-send-email-imammedo@redhat.com -> patchew/1579779525-20065-1-git-send-email-imammedo@redhat.com
>  - [tag update]      patchew/20200124100159.736209-1-stefanha@redhat.com -> patchew/20200124100159.736209-1-stefanha@redhat.com
>  - [tag update]      patchew/20200124162606.8787-1-peter.maydell@linaro.org -> patchew/20200124162606.8787-1-peter.maydell@linaro.org
>  - [tag update]      patchew/20200124172954.28481-1-peter.maydell@linaro.org -> patchew/20200124172954.28481-1-peter.maydell@linaro.org
>  * [new tag]         patchew/20200127103647.17761-1-mlevitsk@redhat.com -> patchew/20200127103647.17761-1-mlevitsk@redhat.com
> Switched to a new branch 'test'
> 9d7e4e6 monitor/hmp: Prefer to use hmp_handle_error for error reporting in block hmp commands
> a549971 add 'error' prefix to vreport
> 7c7da3a monitor: Move hmp_drive_add_node to block-hmp-cmds.c
> a2a0265 monitor/hmp: move hmp_info_block* to block-hmp-cmds.c
> d7f13de monitor/hmp: move remaining hmp_block* functions to block-hmp-cmds.c
> c067499 monitor/hmp: move hmp_nbd_server* to block-hmp-cmds.c
> f5fab94 monitor/hmp: move hmp_snapshot_* to block-hmp-cmds.c
> 4cb26f9 monitor/hmp: move hmp_block_job* to block-hmp-cmds.c
> 97953d5 monitor/hmp: move hmp_drive_mirror and hmp_drive_backup to block-hmp-cmds.c
> 00dc3e8 monitor/hmp: move hmp_drive_del and hmp_commit to block-hmp-cmds.c
> a4aa184 monitor/hmp: rename device-hotplug.c to block/monitor/block-hmp-cmds.c
> d76374a monitor/hmp: uninline add_init_drive
> 13decc9 usb/dev-storage: remove unused include
> 
> === OUTPUT BEGIN ===
> 1/13 Checking commit 13decc9a539d (usb/dev-storage: remove unused include)
> 2/13 Checking commit d76374a8829d (monitor/hmp: uninline add_init_drive)
> 3/13 Checking commit a4aa1842d39b (monitor/hmp: rename device-hotplug.c to block/monitor/block-hmp-cmds.c)
> WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
> #58: 
> new file mode 100644
> 
> total: 0 errors, 1 warnings, 83 lines checked
> 
> Patch 3/13 has style problems, please review.  If any of these errors
> are false positives report them to the maintainer, see
> CHECKPATCH in MAINTAINERS.
> 4/13 Checking commit 00dc3e8c0cc5 (monitor/hmp: move hmp_drive_del and hmp_commit to block-hmp-cmds.c)
> WARNING: Block comments use a leading /* on a separate line
> #81: FILE: block/monitor/block-hmp-cmds.c:119:
> +    /* If this BlockBackend has a device attached to it, its refcount will be
> 
> total: 0 errors, 1 warnings, 234 lines checked
> 
> Patch 4/13 has style problems, please review.  If any of these errors
> are false positives report them to the maintainer, see
> CHECKPATCH in MAINTAINERS.
> 5/13 Checking commit 97953d583c25 (monitor/hmp: move hmp_drive_mirror and hmp_drive_backup to block-hmp-cmds.c)
> 6/13 Checking commit 4cb26f9c9af1 (monitor/hmp: move hmp_block_job* to block-hmp-cmds.c)
> 7/13 Checking commit f5fab9454aca (monitor/hmp: move hmp_snapshot_* to block-hmp-cmds.c)
> WARNING: Block comments use a leading /* on a separate line
> #29: FILE: block/monitor/block-hmp-cmds.c:294:
> +        /* In the future, if 'snapshot-file' is not specified, the snapshot
> 
> WARNING: Block comments use * on subsequent lines
> #30: FILE: block/monitor/block-hmp-cmds.c:295:
> +        /* In the future, if 'snapshot-file' is not specified, the snapshot
> +           will be taken internally. Today it's actually required. */
> 
> WARNING: Block comments use a trailing */ on a separate line
> #30: FILE: block/monitor/block-hmp-cmds.c:295:
> +           will be taken internally. Today it's actually required. */
> 
> total: 0 errors, 3 warnings, 120 lines checked
> 
> Patch 7/13 has style problems, please review.  If any of these errors
> are false positives report them to the maintainer, see
> CHECKPATCH in MAINTAINERS.
> 8/13 Checking commit c06749935535 (monitor/hmp: move hmp_nbd_server* to block-hmp-cmds.c)
> WARNING: Block comments use a leading /* on a separate line
> #60: FILE: block/monitor/block-hmp-cmds.c:363:
> +    /* Then try adding all block devices.  If one fails, close all and
> 
> total: 0 errors, 1 warnings, 217 lines checked
> 
> Patch 8/13 has style problems, please review.  If any of these errors
> are false positives report them to the maintainer, see
> CHECKPATCH in MAINTAINERS.
> 9/13 Checking commit d7f13ded16b4 (monitor/hmp: move remaining hmp_block* functions to block-hmp-cmds.c)
> WARNING: Block comments use a leading /* on a separate line
> #75: FILE: block/monitor/block-hmp-cmds.c:468:
> +    /* qmp_block_set_io_throttle has separate parameters for the
> 
> WARNING: Block comments use a trailing */ on a separate line
> #77: FILE: block/monitor/block-hmp-cmds.c:470:
> +     * version has only one, so we must decide which one to pass. */
> 
> ERROR: "foo* bar" should be "foo *bar"
> #105: FILE: block/monitor/block-hmp-cmds.c:498:
> +    const char* device = qdict_get_str(qdict, "device");
> 
> ERROR: "foo* bar" should be "foo *bar"
> #106: FILE: block/monitor/block-hmp-cmds.c:499:
> +    const char* command = qdict_get_str(qdict, "command");
> 
> total: 2 errors, 2 warnings, 359 lines checked
> 
> Patch 9/13 has style problems, please review.  If any of these errors
> are false positives report them to the maintainer, see
> CHECKPATCH in MAINTAINERS.
> 
> 10/13 Checking commit a2a0265c0910 (monitor/hmp: move hmp_info_block* to block-hmp-cmds.c)
> WARNING: line over 80 characters
> #67: FILE: block/monitor/block-hmp-cmds.c:593:
> +        if (info->has_io_status && info->io_status != BLOCK_DEVICE_IO_STATUS_OK) {
> 
> WARNING: Block comments use a leading /* on a separate line
> #381: FILE: block/monitor/block-hmp-cmds.c:907:
> +            /* The ID is not guaranteed to be the same on all images, so
> 
> total: 0 errors, 2 warnings, 845 lines checked
> 
> Patch 10/13 has style problems, please review.  If any of these errors
> are false positives report them to the maintainer, see
> CHECKPATCH in MAINTAINERS.
> 11/13 Checking commit 7c7da3a311a3 (monitor: Move hmp_drive_add_node to block-hmp-cmds.c)
> 12/13 Checking commit a549971b349f (add 'error' prefix to vreport)
> ERROR: trailing whitespace
> #51: FILE: tests/qemu-iotests/026.out:6:
> +Event: l1_update; errno: 5; imm: off; once: on; write $
> 
> ERROR: trailing whitespace
> #62: FILE: tests/qemu-iotests/026.out:16:
> +Event: l1_update; errno: 5; imm: off; once: off; write $
> 
> ERROR: trailing whitespace
> #79: FILE: tests/qemu-iotests/026.out:30:
> +Event: l1_update; errno: 28; imm: off; once: on; write $
> 
> ERROR: trailing whitespace
> #90: FILE: tests/qemu-iotests/026.out:40:
> +Event: l1_update; errno: 28; imm: off; once: off; write $
> 
> ERROR: trailing whitespace
> #107: FILE: tests/qemu-iotests/026.out:54:
> +Event: l2_load; errno: 5; imm: off; once: on; write $
> 
> ERROR: trailing whitespace
> #116: FILE: tests/qemu-iotests/026.out:70:
> +Event: l2_load; errno: 5; imm: off; once: off; write $
> 
> ERROR: trailing whitespace
> #125: FILE: tests/qemu-iotests/026.out:86:
> +Event: l2_load; errno: 28; imm: off; once: on; write $
> 
> ERROR: trailing whitespace
> #134: FILE: tests/qemu-iotests/026.out:102:
> +Event: l2_load; errno: 28; imm: off; once: off; write $
> 
> ERROR: trailing whitespace
> #143: FILE: tests/qemu-iotests/026.out:118:
> +Event: l2_update; errno: 5; imm: off; once: on; write $
> 
> ERROR: trailing whitespace
> #154: FILE: tests/qemu-iotests/026.out:128:
> +Event: l2_update; errno: 5; imm: off; once: off; write $
> 
> ERROR: trailing whitespace
> #171: FILE: tests/qemu-iotests/026.out:142:
> +Event: l2_update; errno: 28; imm: off; once: on; write $
> 
> ERROR: trailing whitespace
> #182: FILE: tests/qemu-iotests/026.out:152:
> +Event: l2_update; errno: 28; imm: off; once: off; write $
> 
> ERROR: trailing whitespace
> #199: FILE: tests/qemu-iotests/026.out:166:
> +Event: l2_alloc_write; errno: 5; imm: off; once: on; write $
> 
> ERROR: trailing whitespace
> #210: FILE: tests/qemu-iotests/026.out:176:
> +Event: l2_alloc_write; errno: 5; imm: off; once: off; write $
> 
> ERROR: trailing whitespace
> #227: FILE: tests/qemu-iotests/026.out:190:
> +Event: l2_alloc_write; errno: 28; imm: off; once: on; write $
> 
> ERROR: trailing whitespace
> #238: FILE: tests/qemu-iotests/026.out:200:
> +Event: l2_alloc_write; errno: 28; imm: off; once: off; write $
> 
> ERROR: trailing whitespace
> #255: FILE: tests/qemu-iotests/026.out:214:
> +Event: write_aio; errno: 5; imm: off; once: on; write $
> 
> ERROR: trailing whitespace
> #266: FILE: tests/qemu-iotests/026.out:224:
> +Event: write_aio; errno: 5; imm: off; once: off; write $
> 
> ERROR: trailing whitespace
> #283: FILE: tests/qemu-iotests/026.out:238:
> +Event: write_aio; errno: 28; imm: off; once: on; write $
> 
> ERROR: trailing whitespace
> #294: FILE: tests/qemu-iotests/026.out:248:
> +Event: write_aio; errno: 28; imm: off; once: off; write $
> 
> ERROR: trailing whitespace
> #311: FILE: tests/qemu-iotests/026.out:262:
> +Event: refblock_load; errno: 5; imm: off; once: on; write $
> 
> ERROR: trailing whitespace
> #322: FILE: tests/qemu-iotests/026.out:272:
> +Event: refblock_load; errno: 5; imm: off; once: off; write $
> 
> ERROR: trailing whitespace
> #339: FILE: tests/qemu-iotests/026.out:286:
> +Event: refblock_load; errno: 28; imm: off; once: on; write $
> 
> ERROR: trailing whitespace
> #350: FILE: tests/qemu-iotests/026.out:296:
> +Event: refblock_load; errno: 28; imm: off; once: off; write $
> 
> ERROR: trailing whitespace
> #367: FILE: tests/qemu-iotests/026.out:310:
> +Event: refblock_update_part; errno: 5; imm: off; once: on; write $
> 
> ERROR: trailing whitespace
> #378: FILE: tests/qemu-iotests/026.out:320:
> +Event: refblock_update_part; errno: 5; imm: off; once: off; write $
> 
> ERROR: trailing whitespace
> #395: FILE: tests/qemu-iotests/026.out:334:
> +Event: refblock_update_part; errno: 28; imm: off; once: on; write $
> 
> ERROR: trailing whitespace
> #406: FILE: tests/qemu-iotests/026.out:344:
> +Event: refblock_update_part; errno: 28; imm: off; once: off; write $
> 
> ERROR: trailing whitespace
> #423: FILE: tests/qemu-iotests/026.out:358:
> +Event: refblock_alloc; errno: 5; imm: off; once: on; write $
> 
> ERROR: trailing whitespace
> #434: FILE: tests/qemu-iotests/026.out:368:
> +Event: refblock_alloc; errno: 5; imm: off; once: off; write $
> 
> ERROR: trailing whitespace
> #451: FILE: tests/qemu-iotests/026.out:382:
> +Event: refblock_alloc; errno: 28; imm: off; once: on; write $
> 
> ERROR: trailing whitespace
> #462: FILE: tests/qemu-iotests/026.out:392:
> +Event: refblock_alloc; errno: 28; imm: off; once: off; write $
> 
> ERROR: trailing whitespace
> #479: FILE: tests/qemu-iotests/026.out:406:
> +Event: cluster_alloc; errno: 5; imm: off; once: on; write $
> 
> ERROR: trailing whitespace
> #488: FILE: tests/qemu-iotests/026.out:416:
> +Event: cluster_alloc; errno: 5; imm: off; once: off; write $
> 
> ERROR: trailing whitespace
> #497: FILE: tests/qemu-iotests/026.out:426:
> +Event: cluster_alloc; errno: 28; imm: off; once: on; write $
> 
> ERROR: trailing whitespace
> #506: FILE: tests/qemu-iotests/026.out:436:
> +Event: cluster_alloc; errno: 28; imm: off; once: off; write $
> 
> ERROR: trailing whitespace
> #515: FILE: tests/qemu-iotests/026.out:449:
> +Event: refblock_alloc_hookup; errno: 28; imm: off; once: on; write $
> 
> ERROR: trailing whitespace
> #526: FILE: tests/qemu-iotests/026.out:459:
> +Event: refblock_alloc_hookup; errno: 28; imm: off; once: off; write $
> 
> ERROR: trailing whitespace
> #543: FILE: tests/qemu-iotests/026.out:473:
> +Event: refblock_alloc_write; errno: 28; imm: off; once: on; write $
> 
> ERROR: trailing whitespace
> #554: FILE: tests/qemu-iotests/026.out:483:
> +Event: refblock_alloc_write; errno: 28; imm: off; once: off; write $
> 
> ERROR: trailing whitespace
> #571: FILE: tests/qemu-iotests/026.out:497:
> +Event: refblock_alloc_write_blocks; errno: 28; imm: off; once: on; write $
> 
> ERROR: trailing whitespace
> #582: FILE: tests/qemu-iotests/026.out:507:
> +Event: refblock_alloc_write_blocks; errno: 28; imm: off; once: off; write $
> 
> ERROR: trailing whitespace
> #599: FILE: tests/qemu-iotests/026.out:521:
> +Event: refblock_alloc_write_table; errno: 28; imm: off; once: on; write $
> 
> ERROR: trailing whitespace
> #610: FILE: tests/qemu-iotests/026.out:531:
> +Event: refblock_alloc_write_table; errno: 28; imm: off; once: off; write $
> 
> ERROR: trailing whitespace
> #627: FILE: tests/qemu-iotests/026.out:545:
> +Event: refblock_alloc_switch_table; errno: 28; imm: off; once: on; write $
> 
> ERROR: trailing whitespace
> #638: FILE: tests/qemu-iotests/026.out:555:
> +Event: refblock_alloc_switch_table; errno: 28; imm: off; once: off; write $
> 
> ERROR: trailing whitespace
> #781: FILE: tests/qemu-iotests/049.out:107:
> +qemu-img: error: Invalid image size specified! You may use k, M, G, T, P or E suffixes for $
> 
> ERROR: trailing whitespace
> #793: FILE: tests/qemu-iotests/049.out:116:
> +qemu-img: error: Invalid image size specified! You may use k, M, G, T, P or E suffixes for $
> 
> total: 48 errors, 0 warnings, 3053 lines checked
> 
> Patch 12/13 has style problems, please review.  If any of these errors
> are false positives report them to the maintainer, see
> CHECKPATCH in MAINTAINERS.
> 
> 13/13 Checking commit 9d7e4e6ebe5d (monitor/hmp: Prefer to use hmp_handle_error for error reporting in block hmp commands)
> === OUTPUT END ===
> 
> Test command exited with code: 1
> 
> 
> The full log is available at
> http://patchew.org/logs/20200127103647.17761-1-mlevitsk@redhat.com/testing.checkpatch/?type=message.
> ---
> Email generated automatically by Patchew [https://patchew.org/].
> Please send your feedback to patchew-devel@redhat.com
John Snow Jan. 27, 2020, 7:39 p.m. UTC | #3 
On 1/27/20 5:36 AM, Maxim Levitsky wrote:
> This patch series is bunch of cleanups
> to the hmp monitor code.
> 
> This series only touched blockdev related hmp handlers.
> 
> No functional changes expected other that
> light error message changes by the last patch.
> 
> This was inspired by this bugzilla:
> https://bugzilla.redhat.com/show_bug.cgi?id=1719169
> 
> Basically some users still parse hmp error messages,
> and they would like to have them prefixed with 'Error:'
> 

HMP isn't meant to be parsed. It's explicitly *not* API or ABI. I do
like consistency in my UIs (it's useful for human eyes, too), but I'd
like to know more about the request.

Is this request coming from libvirt? Can we wean them off of this
interface? What do they need as a replacement?

(Is blockdev not enough?)

--js
Peter Krempa Jan. 27, 2020, 8:43 p.m. UTC | #4 
On Mon, Jan 27, 2020 at 14:39:02 -0500, John Snow wrote:
> 
> 
> On 1/27/20 5:36 AM, Maxim Levitsky wrote:
> > This patch series is bunch of cleanups
> > to the hmp monitor code.
> > 
> > This series only touched blockdev related hmp handlers.
> > 
> > No functional changes expected other that
> > light error message changes by the last patch.
> > 
> > This was inspired by this bugzilla:
> > https://bugzilla.redhat.com/show_bug.cgi?id=1719169
> > 
> > Basically some users still parse hmp error messages,
> > and they would like to have them prefixed with 'Error:'
> > 
> 
> HMP isn't meant to be parsed. It's explicitly *not* API or ABI. I do
> like consistency in my UIs (it's useful for human eyes, too), but I'd
> like to know more about the request.

That's true as long as there's an stable replacement ... see below.

> 
> Is this request coming from libvirt? Can we wean them off of this
> interface? What do they need as a replacement?

There are 5 commands that libvirt still has HMP interfaces for:

drive_add
drive_del

savevm
loadvm
delvm

From upstream point of view there's no value in adding the 'error'
prefix to drive_add/drive_del as libvirt now uses blockdev-add/del QMP
command instead which have implicit error propagation.

There are no replacements for the internal snapshot commands, but they
reported the 'error' prefix for some time even before this series.

Said that, please don't break savevm/loadvm/delvm until a QMP
replacement is added.

The bug was reported at the time when libvirt didn't use blockdev yet,
but at this point it's pointless from our side. This wouldn't even fix
the scenario when old (pre-5.10) libvirt would use new qemu because the
drive-add handler never checked the error prefix.

[1] https://libvirt.org/git/?p=libvirt.git;a=blob;f=src/qemu/qemu_monitor_text.c;h=9135a11f0a3aae718c86bb199112fba8d16d4d80;hb=HEAD
John Snow Jan. 27, 2020, 9:01 p.m. UTC | #5 
On 1/27/20 3:43 PM, Peter Krempa wrote:
> On Mon, Jan 27, 2020 at 14:39:02 -0500, John Snow wrote:
>>
>>
>> On 1/27/20 5:36 AM, Maxim Levitsky wrote:
>>> This patch series is bunch of cleanups
>>> to the hmp monitor code.
>>>
>>> This series only touched blockdev related hmp handlers.
>>>
>>> No functional changes expected other that
>>> light error message changes by the last patch.
>>>
>>> This was inspired by this bugzilla:
>>> https://bugzilla.redhat.com/show_bug.cgi?id=1719169
>>>
>>> Basically some users still parse hmp error messages,
>>> and they would like to have them prefixed with 'Error:'
>>>
>>
>> HMP isn't meant to be parsed. It's explicitly *not* API or ABI. I do
>> like consistency in my UIs (it's useful for human eyes, too), but I'd
>> like to know more about the request.
> 
> That's true as long as there's an stable replacement ... see below.
> 

Thanks for the context!

>>
>> Is this request coming from libvirt? Can we wean them off of this
>> interface? What do they need as a replacement?
> 
> There are 5 commands that libvirt still has HMP interfaces for:
> 
> drive_add
> drive_del
> 
> savevm
> loadvm
> delvm
> 
> From upstream point of view there's no value in adding the 'error'
> prefix to drive_add/drive_del as libvirt now uses blockdev-add/del QMP
> command instead which have implicit error propagation.
> 

As thought.

> There are no replacements for the internal snapshot commands, but they
> reported the 'error' prefix for some time even before this series.
> 
> Said that, please don't break savevm/loadvm/delvm until a QMP
> replacement is added.
> 

Yes, noted. I wonder where userfaultfd write support is these days...

> The bug was reported at the time when libvirt didn't use blockdev yet,
> but at this point it's pointless from our side. This wouldn't even fix
> the scenario when old (pre-5.10) libvirt would use new qemu because the
> drive-add handler never checked the error prefix.
> 
> [1] https://libvirt.org/git/?p=libvirt.git;a=blob;f=src/qemu/qemu_monitor_text.c;h=9135a11f0a3aae718c86bb199112fba8d16d4d80;hb=HEAD
> 

Thank you for the report from libvirtville :)

--js
Markus Armbruster Jan. 28, 2020, 8:17 a.m. UTC | #6 
Peter Krempa <pkrempa@redhat.com> writes:

> On Mon, Jan 27, 2020 at 14:39:02 -0500, John Snow wrote:
>> 
>> 
>> On 1/27/20 5:36 AM, Maxim Levitsky wrote:
>> > This patch series is bunch of cleanups
>> > to the hmp monitor code.
>> > 
>> > This series only touched blockdev related hmp handlers.
>> > 
>> > No functional changes expected other that
>> > light error message changes by the last patch.
>> > 
>> > This was inspired by this bugzilla:
>> > https://bugzilla.redhat.com/show_bug.cgi?id=1719169
>> > 
>> > Basically some users still parse hmp error messages,
>> > and they would like to have them prefixed with 'Error:'
>> > 
>> 
>> HMP isn't meant to be parsed. It's explicitly *not* API or ABI. I do
>> like consistency in my UIs (it's useful for human eyes, too), but I'd
>> like to know more about the request.
>
> That's true as long as there's an stable replacement ... see below.
>
>> 
>> Is this request coming from libvirt? Can we wean them off of this
>> interface? What do they need as a replacement?
>
> There are 5 commands that libvirt still has HMP interfaces for:
>
> drive_add
> drive_del
>
> savevm
> loadvm
> delvm
>
> From upstream point of view there's no value in adding the 'error'
> prefix to drive_add/drive_del as libvirt now uses blockdev-add/del QMP
> command instead which have implicit error propagation.
>
> There are no replacements for the internal snapshot commands, but they
> reported the 'error' prefix for some time even before this series.
>
> Said that, please don't break savevm/loadvm/delvm until a QMP
> replacement is added.

Replacements have been proposed several times, but they went nowhere
because they replicated the HMP commands' design flaws in QMP.  Here's
one that got some design analysis in its review, by yours truly:
https://lists.nongnu.org/archive/html/qemu-devel/2016-01/msg03593.html

Sane QMP replacements are certainly possible, but so far nobody has
cared enough to pitch in the work.

> The bug was reported at the time when libvirt didn't use blockdev yet,
> but at this point it's pointless from our side. This wouldn't even fix
> the scenario when old (pre-5.10) libvirt would use new qemu because the
> drive-add handler never checked the error prefix.
>
> [1] https://libvirt.org/git/?p=libvirt.git;a=blob;f=src/qemu/qemu_monitor_text.c;h=9135a11f0a3aae718c86bb199112fba8d16d4d80;hb=HEAD

Thanks!
Ján Tomko Jan. 28, 2020, 9:13 a.m. UTC | #7 
On Mon, Jan 27, 2020 at 04:01:31PM -0500, John Snow wrote:
>
>
>On 1/27/20 3:43 PM, Peter Krempa wrote:
>> On Mon, Jan 27, 2020 at 14:39:02 -0500, John Snow wrote:
>>>
>>>
>>> On 1/27/20 5:36 AM, Maxim Levitsky wrote:
>>>> This patch series is bunch of cleanups
>>>> to the hmp monitor code.
>>>>
>>>> This series only touched blockdev related hmp handlers.
>>>>
>>>> No functional changes expected other that
>>>> light error message changes by the last patch.
>>>>
>>>> This was inspired by this bugzilla:
>>>> https://bugzilla.redhat.com/show_bug.cgi?id=1719169
>>>>
>>>> Basically some users still parse hmp error messages,
>>>> and they would like to have them prefixed with 'Error:'
>>>>
>>>

[...]

>> The bug was reported at the time when libvirt didn't use blockdev yet,
>> but at this point it's pointless from our side.

Just for the record, this bug was the motivation for the prefix request:
https://bugzilla.redhat.com/show_bug.cgi?id=1718255

>> This wouldn't even fix
>> the scenario when old (pre-5.10) libvirt would use new qemu because the
>> drive-add handler never checked the error prefix.

And running older libvirt with newer QEMU is not something we care
about.

Jano

>>
>> [1] https://libvirt.org/git/?p=libvirt.git;a=blob;f=src/qemu/qemu_monitor_text.c;h=9135a11f0a3aae718c86bb199112fba8d16d4d80;hb=HEAD
>>
>
>Thank you for the report from libvirtville :)
>
>--js
>
>
Dr. David Alan Gilbert Jan. 28, 2020, 4:47 p.m. UTC | #8 
* John Snow (jsnow@redhat.com) wrote:
> 
> 
> On 1/27/20 3:43 PM, Peter Krempa wrote:
> > On Mon, Jan 27, 2020 at 14:39:02 -0500, John Snow wrote:
> >>
> >>
> >> On 1/27/20 5:36 AM, Maxim Levitsky wrote:
> >>> This patch series is bunch of cleanups
> >>> to the hmp monitor code.
> >>>
> >>> This series only touched blockdev related hmp handlers.
> >>>
> >>> No functional changes expected other that
> >>> light error message changes by the last patch.
> >>>
> >>> This was inspired by this bugzilla:
> >>> https://bugzilla.redhat.com/show_bug.cgi?id=1719169
> >>>
> >>> Basically some users still parse hmp error messages,
> >>> and they would like to have them prefixed with 'Error:'
> >>>
> >>
> >> HMP isn't meant to be parsed. It's explicitly *not* API or ABI. I do
> >> like consistency in my UIs (it's useful for human eyes, too), but I'd
> >> like to know more about the request.
> > 
> > That's true as long as there's an stable replacement ... see below.
> > 
> 
> Thanks for the context!
> 
> >>
> >> Is this request coming from libvirt? Can we wean them off of this
> >> interface? What do they need as a replacement?
> > 
> > There are 5 commands that libvirt still has HMP interfaces for:
> > 
> > drive_add
> > drive_del
> > 
> > savevm
> > loadvm
> > delvm
> > 
> > From upstream point of view there's no value in adding the 'error'
> > prefix to drive_add/drive_del as libvirt now uses blockdev-add/del QMP
> > command instead which have implicit error propagation.
> > 
> 
> As thought.
> 
> > There are no replacements for the internal snapshot commands, but they
> > reported the 'error' prefix for some time even before this series.
> > 
> > Said that, please don't break savevm/loadvm/delvm until a QMP
> > replacement is added.
> > 
> 
> Yes, noted. I wonder where userfaultfd write support is these days...

How would that help you there?

Dave

> > The bug was reported at the time when libvirt didn't use blockdev yet,
> > but at this point it's pointless from our side. This wouldn't even fix
> > the scenario when old (pre-5.10) libvirt would use new qemu because the
> > drive-add handler never checked the error prefix.
> > 
> > [1] https://libvirt.org/git/?p=libvirt.git;a=blob;f=src/qemu/qemu_monitor_text.c;h=9135a11f0a3aae718c86bb199112fba8d16d4d80;hb=HEAD
> > 
> 
> Thank you for the report from libvirtville :)
> 
> --js
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
John Snow Feb. 5, 2020, 11:25 p.m. UTC | #9 
On 1/28/20 11:47 AM, Dr. David Alan Gilbert wrote:
> * John Snow (jsnow@redhat.com) wrote:
>>
>>
>> On 1/27/20 3:43 PM, Peter Krempa wrote:
>>> On Mon, Jan 27, 2020 at 14:39:02 -0500, John Snow wrote:
>>>>
>>>>
>>>> On 1/27/20 5:36 AM, Maxim Levitsky wrote:
>>>>> This patch series is bunch of cleanups
>>>>> to the hmp monitor code.
>>>>>
>>>>> This series only touched blockdev related hmp handlers.
>>>>>
>>>>> No functional changes expected other that
>>>>> light error message changes by the last patch.
>>>>>
>>>>> This was inspired by this bugzilla:
>>>>> https://bugzilla.redhat.com/show_bug.cgi?id=1719169
>>>>>
>>>>> Basically some users still parse hmp error messages,
>>>>> and they would like to have them prefixed with 'Error:'
>>>>>
>>>>
>>>> HMP isn't meant to be parsed. It's explicitly *not* API or ABI. I do
>>>> like consistency in my UIs (it's useful for human eyes, too), but I'd
>>>> like to know more about the request.
>>>
>>> That's true as long as there's an stable replacement ... see below.
>>>
>>
>> Thanks for the context!
>>
>>>>
>>>> Is this request coming from libvirt? Can we wean them off of this
>>>> interface? What do they need as a replacement?
>>>
>>> There are 5 commands that libvirt still has HMP interfaces for:
>>>
>>> drive_add
>>> drive_del
>>>
>>> savevm
>>> loadvm
>>> delvm
>>>
>>> From upstream point of view there's no value in adding the 'error'
>>> prefix to drive_add/drive_del as libvirt now uses blockdev-add/del QMP
>>> command instead which have implicit error propagation.
>>>
>>
>> As thought.
>>
>>> There are no replacements for the internal snapshot commands, but they
>>> reported the 'error' prefix for some time even before this series.
>>>
>>> Said that, please don't break savevm/loadvm/delvm until a QMP
>>> replacement is added.
>>>
>>
>> Yes, noted. I wonder where userfaultfd write support is these days...
> 
> How would that help you there?
> 

Left at the traffic lights, but there was a thought that we'd be able to
get transactionable save-memory support in QMP if we could use
userfaultfd to do just-in-time copies of memory as needed, until the job
is complete.

This way we could support it properly in QMP and we'd have a replacement
for the HMP version which -- from memory -- is not appropriate for the
QMP channel.

Maybe I imagined this restriction.

--js
Dr. David Alan Gilbert Feb. 6, 2020, 9:35 a.m. UTC | #10 
* John Snow (jsnow@redhat.com) wrote:
> 
> 
> On 1/28/20 11:47 AM, Dr. David Alan Gilbert wrote:
> > * John Snow (jsnow@redhat.com) wrote:
> >>
> >>
> >> On 1/27/20 3:43 PM, Peter Krempa wrote:
> >>> On Mon, Jan 27, 2020 at 14:39:02 -0500, John Snow wrote:
> >>>>
> >>>>
> >>>> On 1/27/20 5:36 AM, Maxim Levitsky wrote:
> >>>>> This patch series is bunch of cleanups
> >>>>> to the hmp monitor code.
> >>>>>
> >>>>> This series only touched blockdev related hmp handlers.
> >>>>>
> >>>>> No functional changes expected other that
> >>>>> light error message changes by the last patch.
> >>>>>
> >>>>> This was inspired by this bugzilla:
> >>>>> https://bugzilla.redhat.com/show_bug.cgi?id=1719169
> >>>>>
> >>>>> Basically some users still parse hmp error messages,
> >>>>> and they would like to have them prefixed with 'Error:'
> >>>>>
> >>>>
> >>>> HMP isn't meant to be parsed. It's explicitly *not* API or ABI. I do
> >>>> like consistency in my UIs (it's useful for human eyes, too), but I'd
> >>>> like to know more about the request.
> >>>
> >>> That's true as long as there's an stable replacement ... see below.
> >>>
> >>
> >> Thanks for the context!
> >>
> >>>>
> >>>> Is this request coming from libvirt? Can we wean them off of this
> >>>> interface? What do they need as a replacement?
> >>>
> >>> There are 5 commands that libvirt still has HMP interfaces for:
> >>>
> >>> drive_add
> >>> drive_del
> >>>
> >>> savevm
> >>> loadvm
> >>> delvm
> >>>
> >>> From upstream point of view there's no value in adding the 'error'
> >>> prefix to drive_add/drive_del as libvirt now uses blockdev-add/del QMP
> >>> command instead which have implicit error propagation.
> >>>
> >>
> >> As thought.
> >>
> >>> There are no replacements for the internal snapshot commands, but they
> >>> reported the 'error' prefix for some time even before this series.
> >>>
> >>> Said that, please don't break savevm/loadvm/delvm until a QMP
> >>> replacement is added.
> >>>
> >>
> >> Yes, noted. I wonder where userfaultfd write support is these days...
> > 
> > How would that help you there?
> > 
> 
> Left at the traffic lights, but there was a thought that we'd be able to
> get transactionable save-memory support in QMP if we could use
> userfaultfd to do just-in-time copies of memory as needed, until the job
> is complete.
> 
> This way we could support it properly in QMP and we'd have a replacement
> for the HMP version which -- from memory -- is not appropriate for the
> QMP channel.
> 
> Maybe I imagined this restriction.

The restriction is there; but it's not related to the memory saving;
savevm mostly uses the core migration code (which would normally run in
a separate thread) but uses it itself in it's own loop in the main
thread with a bunch of bdrv code glued around it to do the snapshots
there. 
It shouldn't be that hard to convert it to be much more like a normal
migration - except it needs some hook then to do the snapshotting stuff
at the end.

Dave

> --js
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK