[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 8/9] monitor: move hmp_info_block* to blockdev-hmp-cmds.c
From: |
Markus Armbruster |
Subject: |
Re: [PATCH 8/9] monitor: move hmp_info_block* to blockdev-hmp-cmds.c |
Date: |
Mon, 27 Jan 2020 14:33:11 +0100 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/26.3 (gnu/linux) |
Maxim Levitsky <address@hidden> 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!