Re: [PATCH 8/9] monitor: move hmp_info_block* to blockdev-hmp-cmds.c
Am 27.01.2020 um 14:33 hat Markus Armbruster geschrieben: > Maxim Levitsky 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? Moving it to block is not an obvious improvement, so I think I'd leave it alone. Kevin
Re: [PATCH 8/9] monitor: move hmp_info_block* to blockdev-hmp-cmds.c
On Mon, 2020-01-27 at 14:33 +0100, Markus Armbruster wrote:
> Maxim Levitsky 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
Re: [PATCH 8/9] monitor: move hmp_info_block* to blockdev-hmp-cmds.c
Maxim Levitsky 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!
Re: [PATCH 8/9] monitor: move hmp_info_block* to blockdev-hmp-cmds.c
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 > 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. > 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 ? Best regards and thanks for the detailed review! Maxim Levitsky
Re: [PATCH 8/9] monitor: move hmp_info_block* to blockdev-hmp-cmds.c
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(). 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 cpus.c dump/dump.c hw/display/qxl.c hw/scsi/vhost-scsi.c hw/usb/dev-storage.c include/monitor/monitor.h migration/migration.c monitor/hmp-cmds.c monitor/hmp.c monitor/misc.c monitor/qmp-cmds.c qdev-monitor.c vl.c
