Re: [Qemu-devel] [RESEND PATCH v4 1/6] acpi, mem-hotplug: Add acpi_memory_slot_status() to get MemStatus

[Zhu Guihua]

On 03/16/2015 09:58 PM, Michael S. Tsirkin wrote:
> On Mon, Mar 16, 2015 at 04:58:13PM +0800, Zhu Guihua wrote:
> > From: Tang Chen <address@hidden>
> >
> > Add a new API named acpi_memory_slot_status() to obtain a single memory
> > slot status. Doing this is because this procedure will be used by other
> > functions in the next coming patches.
> >
> > Signed-off-by: Tang Chen <address@hidden>
> > Signed-off-by: Zhu Guihua <address@hidden>
> It's generally only OK to add API in a separate
> patch from its use if the API is well documented.
> There's 0 documentation here, so please just squash this
> with the user so one can review without jumping
> back and forth.

The new API will be used in several places. If I squash this with the
user, I think the patch will be too large.

So I think it is better to add documentation here. what's your opinion?

Thanks,
Zhu

> > ---
> >   hw/acpi/memory_hotplug.c | 24 ++++++++++++++++++------
> >   1 file changed, 18 insertions(+), 6 deletions(-)
> >
> > diff --git a/hw/acpi/memory_hotplug.c b/hw/acpi/memory_hotplug.c
> > index c6580da..0efc357 100644
> > --- a/hw/acpi/memory_hotplug.c
> > +++ b/hw/acpi/memory_hotplug.c
> > @@ -163,29 +163,41 @@ void acpi_memory_hotplug_init(MemoryRegion *as, Object 
> > *owner,
> >       memory_region_add_subregion(as, ACPI_MEMORY_HOTPLUG_BASE, &state->io);
> >   }
> >   
> > -void acpi_memory_plug_cb(ACPIREGS *ar, qemu_irq irq, MemHotplugState *mem_st,
> > -                         DeviceState *dev, Error **errp)
> > +static MemStatus *
> > +acpi_memory_slot_status(MemHotplugState *mem_st,
> > +                        DeviceState *dev, Error **errp)
> >   {
> > -    MemStatus *mdev;
> >       Error *local_err = NULL;
> >       int slot = object_property_get_int(OBJECT(dev), PC_DIMM_SLOT_PROP,
> >                                          &local_err);
> >   
> >       if (local_err) {
> >           error_propagate(errp, local_err);
> > -        return;
> > +        return NULL;
> >       }
> >   
> >       if (slot >= mem_st->dev_count) {
> >           char *dev_path = object_get_canonical_path(OBJECT(dev));
> > -        error_setg(errp, "acpi_memory_plug_cb: "
> > +        error_setg(errp, "acpi_memory_slot_status: "
> >                      "device [%s] returned invalid memory slot[%d]",
> >                       dev_path, slot);
> >           g_free(dev_path);
> > +        return NULL;
> > +    }
> > +
> > +    return &mem_st->devs[slot];
> > +}
> > +
> > +void acpi_memory_plug_cb(ACPIREGS *ar, qemu_irq irq, MemHotplugState *mem_st,
> > +                         DeviceState *dev, Error **errp)
> > +{
> > +    MemStatus *mdev;
> > +
> > +    mdev = acpi_memory_slot_status(mem_st, dev, errp);
> > +    if (!mdev) {
> >           return;
> >       }
> >   
> > -    mdev = &mem_st->devs[slot];
> >       mdev->dimm = dev;
> >       mdev->is_enabled = true;
> >       mdev->is_inserting = true;
> > --
> > 1.9.3
> .