Re: [Qemu-devel] [PATCH v2 04/14] pc: Eliminate struct PcGuestInfoState

[Marcel Apfelbaum <marcel@redhat.com>]

On 12/11/2015 08:42 PM, Eduardo Habkost wrote:
> Instead of allocating a new struct just for PcGuestInfo and the
> mchine_done Notifier, place them inside PCMachineState.

^^^^^^^^ "machine_done", ..... ^^ "it"

  it doesn't worth a new version, maybe the maintainer
can take care of it.

>
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>   hw/i386/pc.c         | 27 ++++++++++-----------------
>   include/hw/i386/pc.h |  2 ++
>   2 files changed, 12 insertions(+), 17 deletions(-)
>
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index f32000a..30cdfaf 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1155,18 +1155,12 @@ typedef struct PcRomPciInfo {
>       uint64_t w64_max;
>   } PcRomPciInfo;
>
> -typedef struct PcGuestInfoState {
> -    PcGuestInfo info;
> -    Notifier machine_done;
> -} PcGuestInfoState;
> -
>   static
> -void pc_guest_info_machine_done(Notifier *notifier, void *data)
> +void pc_machine_done(Notifier *notifier, void *data)
>   {
> -    PcGuestInfoState *guest_info_state = container_of(notifier,
> -                                                      PcGuestInfoState,
> -                                                      machine_done);
> -    PCIBus *bus = PC_MACHINE(qdev_get_machine())->bus;
> +    PCMachineState *pcms = container_of(notifier,
> +                                        PCMachineState, machine_done);

good! no need for the ad-hoc qdev_get_machine() query anymore.

> +    PCIBus *bus = pcms->bus;
>
>       if (bus) {
>           int extra_hosts = 0;
> @@ -1177,21 +1171,20 @@ void pc_guest_info_machine_done(Notifier *notifier, void *data)
>                   extra_hosts++;
>               }
>           }
> -        if (extra_hosts && guest_info_state->info.fw_cfg) {
> +        if (extra_hosts && pcms->acpi_guest_info.fw_cfg) {
>               uint64_t *val = g_malloc(sizeof(*val));
>               *val = cpu_to_le64(extra_hosts);
> -            fw_cfg_add_file(guest_info_state->info.fw_cfg,
> +            fw_cfg_add_file(pcms->acpi_guest_info.fw_cfg,
>                       "etc/extra-pci-roots", val, sizeof(*val));
>           }
>       }
>
> -    acpi_setup(&guest_info_state->info);
> +    acpi_setup(&pcms->acpi_guest_info);
>   }
>
>   PcGuestInfo *pc_guest_info_init(PCMachineState *pcms)
>   {
> -    PcGuestInfoState *guest_info_state = g_malloc0(sizeof *guest_info_state);
> -    PcGuestInfo *guest_info = &guest_info_state->info;
> +    PcGuestInfo *guest_info = &pcms->acpi_guest_info;
>       int i, j;
>
>       guest_info->ram_size_below_4g = pcms->below_4g_mem_size;
> @@ -1219,8 +1212,8 @@ PcGuestInfo *pc_guest_info_init(PCMachineState *pcms)
>           }
>       }
>
> -    guest_info_state->machine_done.notify = pc_guest_info_machine_done;
> -    qemu_add_machine_init_done_notifier(&guest_info_state->machine_done);
> +    pcms->machine_done.notify = pc_machine_done;
> +    qemu_add_machine_init_done_notifier(&pcms->machine_done);
>       return guest_info;
>   }
>
> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index 84bc88f..74c0515 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -48,6 +48,8 @@ struct PCMachineState {
>
>       /* State for other subsystems/APIs: */
>       MemoryHotplugState hotplug_memory;
> +    PcGuestInfo acpi_guest_info;

thanks for the clue, we know is some acpi related stuff.
you could get rid off the _guest_ and use  acpi_info,
but since is will be removed anyway there is no need for it.


> +    Notifier machine_done;
>
>       /* Pointers to devices and objects: */
>       HotplugHandler *acpi_dev;
>


Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>