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

[Markus Armbruster <armbru@redhat.com>]

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.

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

>
> Best regards and thanks for the detailed review!
> 	Maxim Levitsky

You're welcome!