Re: [Qemu-devel] [PATCH v4 6/8] qdev: introduce FWPathProvider interface

[Andreas Färber]

Am 11.12.2013 11:22, schrieb Alexey Kardashevskiy:
> QEMU supports firmware names for all devices in the QEMU tree but
> some architectures expect some parts of firmware path names in different
> format.
> 
> This introduces a firmware-pathname-change interface definition.
> If some machines needs to redefine the firmware path format, it has
> to add the TYPE_FW_PATH_PROVIDER interface to an object that is above
> the device on the QOM tree (typically /machine).
> 
> Signed-off-by: Alexey Kardashevskiy <address@hidden>
> Signed-off-by: Paolo Bonzini <address@hidden>
> ---
> Changes:
> v4:
> * added fw-path-provider.o into tests/Makefile
> * fixed 80chars warning from checkpatch.pl

I am puzzled that the patch ends in Paolo's Sob but was sent by Alexey.
Was this picked up from another series or is it even in multiple
concurrent series? Were changes performed since Paolo signed it off? If
not, please provide a Patchwork link to the original for convenience. :)

> ---
>  hw/core/Makefile.objs         |  1 +
>  hw/core/fw-path-provider.c    | 36 ++++++++++++++++++++++++++++++++++++
>  hw/core/qdev.c                | 18 +++++++++++++++++-
>  include/hw/fw-path-provider.h | 32 ++++++++++++++++++++++++++++++++
>  tests/Makefile                |  1 +
>  5 files changed, 87 insertions(+), 1 deletion(-)
>  create mode 100644 hw/core/fw-path-provider.c
>  create mode 100644 include/hw/fw-path-provider.h
> 
> diff --git a/hw/core/Makefile.objs b/hw/core/Makefile.objs
> index 950146c..d829479 100644
> --- a/hw/core/Makefile.objs
> +++ b/hw/core/Makefile.objs
> @@ -1,5 +1,6 @@
>  # core qdev-related obj files, also used by *-user:
>  common-obj-y += qdev.o qdev-properties.o
> +common-obj-y += fw-path-provider.o
>  # irq.o needed for qdev GPIO handling:
>  common-obj-y += irq.o
>  
> diff --git a/hw/core/fw-path-provider.c b/hw/core/fw-path-provider.c
> new file mode 100644
> index 0000000..0187568
> --- /dev/null
> +++ b/hw/core/fw-path-provider.c
> @@ -0,0 +1,36 @@
> +#include "hw/fw-path-provider.h"

This new file is lacking a license header. Same for the header file below.

> +
> +char *fw_path_provider_get_dev_path(FWPathProvider *p, BusState *bus,
> +                                    DeviceState *dev)
> +{
> +    FWPathProviderClass *k = FW_PATH_PROVIDER_GET_CLASS(p);
> +
> +    return k->get_dev_path(p, bus, dev);
> +}
> +
> +char *fw_path_provider_try_get_dev_path(Object *o, BusState *bus,
> +                                        DeviceState *dev)
> +{
> +    FWPathProvider *p = (FWPathProvider *)
> +        object_dynamic_cast(o, TYPE_FW_PATH_PROVIDER);
> +
> +    if (p) {
> +        return fw_path_provider_get_dev_path(p, bus, dev);
> +    }
> +
> +    return NULL;
> +}
> +
> +static const TypeInfo fw_path_provider_info = {
> +    .name          = TYPE_FW_PATH_PROVIDER,
> +    .parent        = TYPE_INTERFACE,
> +    .class_size = sizeof(FWPathProviderClass),

Indentation looks inconsistent.

> +};
> +

Double white line seems unintentional.

> +
> +static void fw_path_provider_register_types(void)
> +{
> +    type_register_static(&fw_path_provider_info);
> +}
> +
> +type_init(fw_path_provider_register_types)
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index 959130c..81a0e75 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -26,6 +26,7 @@
>     this API directly.  */
>  
>  #include "hw/qdev.h"
> +#include "hw/fw-path-provider.h"
>  #include "sysemu/sysemu.h"
>  #include "qapi/error.h"
>  #include "qapi/qmp/qerror.h"
> @@ -503,6 +504,18 @@ static char *bus_get_fw_dev_path(BusState *bus, 
> DeviceState *dev)
>      return NULL;
>  }
>  
> +static char *qdev_get_fw_dev_path_from_handler(BusState *bus, DeviceState 
> *dev)
> +{
> +    Object *obj = OBJECT(dev);
> +    char *d = NULL;
> +
> +    while (!d && obj->parent) {
> +        obj = obj->parent;
> +        d = fw_path_provider_try_get_dev_path(obj, bus, dev);
> +    }
> +    return d;
> +}
> +
>  static int qdev_get_fw_dev_path_helper(DeviceState *dev, char *p, int size)
>  {
>      int l = 0;
> @@ -510,7 +523,10 @@ static int qdev_get_fw_dev_path_helper(DeviceState *dev, 
> char *p, int size)
>      if (dev && dev->parent_bus) {
>          char *d;
>          l = qdev_get_fw_dev_path_helper(dev->parent_bus->parent, p, size);
> -        d = bus_get_fw_dev_path(dev->parent_bus, dev);
> +        d = qdev_get_fw_dev_path_from_handler(dev->parent_bus, dev);
> +        if (!d) {
> +            d = bus_get_fw_dev_path(dev->parent_bus, dev);
> +        }
>          if (d) {
>              l += snprintf(p + l, size - l, "%s", d);
>              g_free(d);
> diff --git a/include/hw/fw-path-provider.h b/include/hw/fw-path-provider.h
> new file mode 100644
> index 0000000..765170b
> --- /dev/null
> +++ b/include/hw/fw-path-provider.h
> @@ -0,0 +1,32 @@
> +#ifndef FW_PATH_PROVIDER_H
> +#define FW_PATH_PROVIDER_H 1
> +
> +#include "qemu-common.h"
> +#include "qom/object.h"
> +
> +#define TYPE_FW_PATH_PROVIDER "fw-path-provider"
> +
> +#define FW_PATH_PROVIDER_CLASS(klass) \
> +     OBJECT_CLASS_CHECK(FWPathProviderClass, (klass), TYPE_FW_PATH_PROVIDER)
> +#define FW_PATH_PROVIDER_GET_CLASS(obj) \
> +    OBJECT_GET_CLASS(FWPathProviderClass, (obj), TYPE_FW_PATH_PROVIDER)
> +#define FW_PATH_PROVIDER(obj) \
> +     INTERFACE_CHECK(FWPathProvider, (obj), TYPE_FW_PATH_PROVIDER)
> +
> +typedef struct FWPathProvider {
> +    Object Parent;

parent_obj

> +} FWPathProvider;
> +
> +typedef void (*StreamCanPushNotifyFn)(void *opaque);
> +
> +typedef struct FWPathProviderClass {
> +    InterfaceClass parent;

parent_class and white line after, please.

Patch looks independent of the other pieces, so we could extract some
non-ppc pieces if this is not just waiting for an ack.
The QOM patches should already be upstream by now.

Thanks,
Andreas

> +    char *(*get_dev_path)(FWPathProvider *p, BusState *bus, DeviceState 
> *dev);
> +} FWPathProviderClass;
> +
> +char *fw_path_provider_get_dev_path(FWPathProvider *p, BusState *bus,
> +                                    DeviceState *dev);
> +char *fw_path_provider_try_get_dev_path(Object *o, BusState *bus,
> +                                        DeviceState *dev);
> +
> +#endif /* FW_PATH_PROVIDER_H */
> diff --git a/tests/Makefile b/tests/Makefile
> index 379cdd9..09c42d0 100644
> --- a/tests/Makefile
> +++ b/tests/Makefile
> @@ -160,6 +160,7 @@ tests/test-qdev-global-props$(EXESUF): 
> tests/test-qdev-global-props.o \
>       hw/core/qdev.o hw/core/qdev-properties.o \
>       hw/core/irq.o \
>       qom/object.o qom/container.o qom/qom-qobject.o \
> +     hw/core/fw-path-provider.o \
>       $(test-qapi-obj-y) \
>       libqemuutil.a libqemustub.a
>  
> 


-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg