[1/3] qdev: Create qdev_get_device_class() function

Extract the DeviceClass lookup from qdev_device_add() to a separate
function.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 qdev-monitor.c | 70 +++++++++++++++++++++++++++++++++++-----------------------
 1 file changed, 42 insertions(+), 28 deletions(-)

Hi,

Am 01.11.2014 um 16:56 schrieb Eduardo Habkost:
> Extract the DeviceClass lookup from qdev_device_add() to a separate
> function.
> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>  qdev-monitor.c | 70 +++++++++++++++++++++++++++++++++++-----------------------
>  1 file changed, 42 insertions(+), 28 deletions(-)
> 
> diff --git a/qdev-monitor.c b/qdev-monitor.c
> index fac7d17..982f3f4 100644
> --- a/qdev-monitor.c
> +++ b/qdev-monitor.c
> @@ -180,6 +180,44 @@ static const char *find_typename_by_alias(const char *alias)
>      return NULL;
>  }
>  
> +static DeviceClass *qdev_get_device_class(const char **driver, Error **errp)

Since this does nothing qdev-specific, what about
device_get_class_by_name()? The only that's not generically suitable for
hw/core/ is the "driver" naming in the error handling; otherwise it
looks very similar to the CPUClass hooks I added a while back.

> +{
> +    ObjectClass *oc;
> +    DeviceClass *dc;
> +
> +    oc = object_class_by_name(*driver);
> +    if (!oc) {
> +        const char *typename = find_typename_by_alias(*driver);
> +
> +        if (typename) {
> +            *driver = typename;
> +            oc = object_class_by_name(*driver);
> +        }
> +    }
> +
> +    if (!object_class_dynamic_cast(oc, TYPE_DEVICE)) {
> +        error_setg(errp, "'%s' is not a valid device model name", *driver);
> +        return NULL;
> +    }
> +
> +    if (object_class_is_abstract(oc)) {
> +        error_set(errp, QERR_INVALID_PARAMETER_VALUE, "driver",
> +                  "non-abstract device type");
> +        return NULL;
> +    }
> +

See 3/3 for whether we may want to return here.

> +    dc = DEVICE_CLASS(oc);
> +    if (dc->cannot_instantiate_with_device_add_yet ||
> +        (qdev_hotplug && !dc->hotpluggable)) {
> +        error_set(errp, QERR_INVALID_PARAMETER_VALUE, "driver",
> +                  "pluggable device type");
> +        return NULL;
> +    }
> +
> +    return dc;
> +}
> +
> +
>  int qdev_device_help(QemuOpts *opts)
>  {
>      Error *local_err = NULL;
> @@ -455,7 +493,6 @@ static BusState *qbus_find(const char *path)
>  
>  DeviceState *qdev_device_add(QemuOpts *opts)
>  {
> -    ObjectClass *oc;
>      DeviceClass *dc;
>      const char *driver, *path, *id;
>      DeviceState *dev;
> @@ -469,33 +506,10 @@ DeviceState *qdev_device_add(QemuOpts *opts)
>      }
>  
>      /* find driver */
> -    oc = object_class_by_name(driver);
> -    if (!oc) {
> -        const char *typename = find_typename_by_alias(driver);
> -
> -        if (typename) {
> -            driver = typename;
> -            oc = object_class_by_name(driver);
> -        }
> -    }
> -
> -    if (!object_class_dynamic_cast(oc, TYPE_DEVICE)) {
> -        qerror_report(ERROR_CLASS_GENERIC_ERROR,
> -                      "'%s' is not a valid device model name", driver);
> -        return NULL;
> -    }
> -
> -    if (object_class_is_abstract(oc)) {
> -        qerror_report(QERR_INVALID_PARAMETER_VALUE, "driver",
> -                      "non-abstract device type");
> -        return NULL;
> -    }
> -
> -    dc = DEVICE_CLASS(oc);
> -    if (dc->cannot_instantiate_with_device_add_yet ||
> -        (qdev_hotplug && !dc->hotpluggable)) {
> -        qerror_report(QERR_INVALID_PARAMETER_VALUE, "driver",
> -                      "pluggable device type");
> +    dc = qdev_get_device_class(&driver, &err);
> +    if (err) {
> +        qerror_report_err(err);
> +        error_free(err);
>          return NULL;
>      }
>  

Otherwise looks good.

Regards,
Andreas

On Sat, Nov 01, 2014 at 05:46:14PM +0100, Andreas Färber wrote:
> Hi,
> 
> Am 01.11.2014 um 16:56 schrieb Eduardo Habkost:
> > Extract the DeviceClass lookup from qdev_device_add() to a separate
> > function.
> > 
> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > ---
> >  qdev-monitor.c | 70 +++++++++++++++++++++++++++++++++++-----------------------
> >  1 file changed, 42 insertions(+), 28 deletions(-)
> > 
> > diff --git a/qdev-monitor.c b/qdev-monitor.c
> > index fac7d17..982f3f4 100644
> > --- a/qdev-monitor.c
> > +++ b/qdev-monitor.c
> > @@ -180,6 +180,44 @@ static const char *find_typename_by_alias(const char *alias)
> >      return NULL;
> >  }
> >  
> > +static DeviceClass *qdev_get_device_class(const char **driver, Error **errp)
> 
> Since this does nothing qdev-specific, what about
> device_get_class_by_name()? The only that's not generically suitable for
> hw/core/ is the "driver" naming in the error handling; otherwise it
> looks very similar to the CPUClass hooks I added a while back.

Well, I don't know where's the line that divides "qdev-specific" from
"DeviceState-specific". The distinction seems arbitrary to me, and I
won't see any problem if somebody moves all qdev-*.c files inside
hw/core.  :)

I assume that at least qdev_alias_table (which is used by both
qdev_device_add() and qdev_device_help()) can be considered
qdev-specific (but just because the table is currently inside
qdev-monitor.c).

Anyway, the only point of this function is to reduce code duplication
between qdev_device_add() and qdev_device_help() while fixing the
"-device ,help" crashes. If somebody wants to move any of the existing
qdev-*.c logic to hw/core later, I won't complain (but I wouldn't want
to do that after hard freeze).

> 
> > +{
> > +    ObjectClass *oc;
> > +    DeviceClass *dc;
> > +
> > +    oc = object_class_by_name(*driver);
> > +    if (!oc) {
> > +        const char *typename = find_typename_by_alias(*driver);
> > +
> > +        if (typename) {
> > +            *driver = typename;
> > +            oc = object_class_by_name(*driver);
> > +        }
> > +    }
> > +
> > +    if (!object_class_dynamic_cast(oc, TYPE_DEVICE)) {
> > +        error_setg(errp, "'%s' is not a valid device model name", *driver);
> > +        return NULL;
> > +    }
> > +
> > +    if (object_class_is_abstract(oc)) {
> > +        error_set(errp, QERR_INVALID_PARAMETER_VALUE, "driver",
> > +                  "non-abstract device type");
> > +        return NULL;
> > +    }
> > +
> 
> See 3/3 for whether we may want to return here.

The point of this function is to allow reuse of checks that are
necessary on both qdev_device_add() and qdev_device_help(), and both
functions need to reject abstract and
cannot_instantiate_with_device_add_yet classes.