[PATCH 0/9] RFC: [for 5.0]: HMP monitor handlers cleanups

Maxim Levitsky posted 9 patches 4 years, 5 months ago
Test asan passed
Test checkpatch passed
Test FreeBSD passed
Test docker-mingw@fedora passed
Test docker-clang@ubuntu passed
Test docker-quick@centos7 passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20191120185850.18986-1-mlevitsk@redhat.com
Maintainers: Max Reitz <mreitz@redhat.com>, Markus Armbruster <armbru@redhat.com>, "Dr. David Alan Gilbert" <dgilbert@redhat.com>, Kevin Wolf <kwolf@redhat.com>
There is a newer version of this series
MAINTAINERS         |   1 +
Makefile.objs       |   4 +-
blockdev-hmp-cmds.c | 656 ++++++++++++++++++++++++++++++++++++++++++++
blockdev.c          |  95 -------
device-hotplug.c    |  91 ------
monitor/hmp-cmds.c  | 465 -------------------------------
6 files changed, 659 insertions(+), 653 deletions(-)
create mode 100644 blockdev-hmp-cmds.c
delete mode 100644 device-hotplug.c
[PATCH 0/9] RFC: [for 5.0]: HMP monitor handlers cleanups
Posted by Maxim Levitsky 4 years, 5 months ago
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.

Best regards,
	Maxim Levitsky

Maxim Levitsky (9):
  monitor: uninline add_init_drive
  monitor: rename device-hotplug.c to blockdev-hmp-cmds.c
  monitor: move hmp_drive_del and hmp_commit to blockdev-hmp-cmds.c
  monitor: move hmp_drive_mirror and hmp_drive_backup to
    blockdev-hmp-cmds.c
  monitor: move hmp_block_job* to blockdev-hmp-cmd.c
  monitor: move hmp_snapshot_* to blockdev-hmp-cmds.c
  monitor: move remaining hmp_block* functions to blockdev-hmp-cmds.c
  monitor: move hmp_info_block* to blockdev-hmp-cmds.c
  monitor/hmp: Prefer to use hmp_handle_error for error reporting in
    block hmp commands

 MAINTAINERS         |   1 +
 Makefile.objs       |   4 +-
 blockdev-hmp-cmds.c | 656 ++++++++++++++++++++++++++++++++++++++++++++
 blockdev.c          |  95 -------
 device-hotplug.c    |  91 ------
 monitor/hmp-cmds.c  | 465 -------------------------------
 6 files changed, 659 insertions(+), 653 deletions(-)
 create mode 100644 blockdev-hmp-cmds.c
 delete mode 100644 device-hotplug.c

-- 
2.17.2


Re: [PATCH 0/9] RFC: [for 5.0]: HMP monitor handlers cleanups
Posted by Dr. David Alan Gilbert 4 years, 4 months ago
* Maxim Levitsky (mlevitsk@redhat.com) wrote:
> This patch series is bunch of cleanups
> to the hmp monitor code.
> 
> This series only touched blockdev related hmp handlers.

Hi Maxim,
  This looks mostly OK to me from the HMP side - with one change;
can you please move the blockdev-hmp-cmds.c into either monitor/ or
block/; with that


Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

we also need a corresponding review by a block person.

Dave


> 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.
> 
> Best regards,
> 	Maxim Levitsky
> 
> Maxim Levitsky (9):
>   monitor: uninline add_init_drive
>   monitor: rename device-hotplug.c to blockdev-hmp-cmds.c
>   monitor: move hmp_drive_del and hmp_commit to blockdev-hmp-cmds.c
>   monitor: move hmp_drive_mirror and hmp_drive_backup to
>     blockdev-hmp-cmds.c
>   monitor: move hmp_block_job* to blockdev-hmp-cmd.c
>   monitor: move hmp_snapshot_* to blockdev-hmp-cmds.c
>   monitor: move remaining hmp_block* functions to blockdev-hmp-cmds.c
>   monitor: move hmp_info_block* to blockdev-hmp-cmds.c
>   monitor/hmp: Prefer to use hmp_handle_error for error reporting in
>     block hmp commands
> 
>  MAINTAINERS         |   1 +
>  Makefile.objs       |   4 +-
>  blockdev-hmp-cmds.c | 656 ++++++++++++++++++++++++++++++++++++++++++++
>  blockdev.c          |  95 -------
>  device-hotplug.c    |  91 ------
>  monitor/hmp-cmds.c  | 465 -------------------------------
>  6 files changed, 659 insertions(+), 653 deletions(-)
>  create mode 100644 blockdev-hmp-cmds.c
>  delete mode 100644 device-hotplug.c
> 
> -- 
> 2.17.2
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK


Re: [PATCH 0/9] RFC: [for 5.0]: HMP monitor handlers cleanups
Posted by Kevin Wolf 4 years, 4 months ago
Am 22.11.2019 um 11:15 hat Dr. David Alan Gilbert geschrieben:
> * Maxim Levitsky (mlevitsk@redhat.com) wrote:
> > This patch series is bunch of cleanups
> > to the hmp monitor code.
> > 
> > This series only touched blockdev related hmp handlers.
> 
> Hi Maxim,
>   This looks mostly OK to me from the HMP side - with one change;
> can you please move the blockdev-hmp-cmds.c into either monitor/ or
> block/; with that

Let's create a block/monitor/hmp.c then. There is more stuff that could
move into that directory later.

Kevin