qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH 1/3] qdev: Create qdev_get_device_class() functi


From: Eduardo Habkost
Subject: Re: [Qemu-devel] [PATCH 1/3] qdev: Create qdev_get_device_class() function
Date: Sat, 1 Nov 2014 15:03:33 -0200
User-agent: Mutt/1.5.23 (2014-03-12)

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 <address@hidden>
> > ---
> >  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.

-- 
Eduardo



reply via email to

[Prev in Thread] Current Thread [Next in Thread]