Re: [PATCH 8/9] monitor: move hmp_info_block* to blockdev-hmp-cmds.c

[Maxim Levitsky <mlevitsk@redhat.com>]

On Mon, 2020-01-27 at 14:33 +0100, Markus Armbruster wrote:
> Maxim Levitsky <mlevitsk@redhat.com> writes:
> 
> > On Wed, 2019-11-27 at 09:08 +0100, Markus Armbruster wrote:
> > > I think it makes sense to collect *all* block HMP stuff here.
> > > 
> > > Left in monitor/hmp-cmds.c: hmp_eject(), hmp_nbd_server_start(), ...
> > > 
> > > I guess hmp_change() has to stay there, because it's both block and ui.
> > > 
> > > Left in blockdev.c: hmp_drive_add_node().
> > 
> > Thank you very much. I added these and bunch more to my patchset.
> > 
> > > 
> > > Quick grep for possible files to check:
> > > 
> > > $ git-grep -l 'monitor[a-z_-]*.h' | xargs grep -l 'block[a-z_-]*\.h'
> > > MAINTAINERS
> > > blockdev-hmp-cmds.c
> > > 
> > > blockdev.c
> > 
> > hmp_drive_add_node is there and I moved it too.
> > 
> > 
> > > cpus.c
> > 
> > Nothing suspicious
> > 
> > > dump/dump.c
> > 
> > qmp_dump_guest_memory is only monitor reference there I think
> > 
> > > hw/display/qxl.c
> > 
> > No way that is related to the block layer
> > 
> > > hw/scsi/vhost-scsi.c
> > 
> > All right, the monitor_fd_param is an interesting thing.
> > Not related to block though.
> > 
> > > hw/usb/dev-storage.c
> > 
> > All right, this for no reason includes monitor/monitor.h,
> > added patch to remove this because why not.
> > 
> > > include/monitor/monitor.h
> > 
> > Nothing suspicious
> > 
> > > migration/migration.c
> > 
> > Nothing suspicious
> > 
> > > monitor/hmp-cmds.c
> > 
> > Added hmp_qemu_io
> > 
> > Maybe I need to add hmp_delvm too?
> > savevm/delvm do old style snapshots
> > which are stored to the first block device
> 
> One foot in the block subsystem, the other foot in the migration
> subsystem.  I'm not sure where this should go.  Kevin?
> 
> > > monitor/hmp.c
> > 
> > There are some block references in monitor_find_completion,
> > but I guess it is not worth it to move that
> > 
> > > monitor/misc.c
> > 
> > vm_completion for delvm/loadvm.
> 
> Having completion close to whatever it completes would be nice, I guess.
> 
> When in doubt, leave the savevm / delvm stuff alone.
Yep.

> 
> > > monitor/qmp-cmds.c
> > 
> > Nothing hmp related at first glance.
> > 
> > > qdev-monitor.c
> > 
> > blk_by_qdev_id - used by both hmp and qmp code
> > 
> > > vl.c
> > 
> > Hopefully nothing hmp+block related, I searched the file for
> > few things but I can't be fully sure.
> > Out of the curiosity do you know why this file is called like that,
> > since it hosts qemu main(), shouldn't it be called main.c ?
> 
> Its first commit 0824d6fc67 "for hard core developpers only: a new user
> mode linux project :-)" calls the executable "vl", and has
> 
>     void help(void)
>     {
>         printf("Virtual Linux version " QEMU_VERSION ", Copyright (c) 2003 Fabrice Bellard\n"
>                "usage: vl [-h] bzImage initrd [kernel parameters...]\n"
>                "\n"
>     [...]
>         exit(1);
>     }
> 
> The executable was renamed soon after.  I guess the source file name has
> made people wonder ever since.
Nice :-)

> 
> > 
> > Best regards and thanks for the detailed review!
> > 	Maxim Levitsky
> 
> You're welcome!

I hope we can move forward with this patch series as well.

Best regards,
	Maxim Levitsky