Re: [RFC 2/5] qom: allow failure of object_new_with_class

["Daniel P. Berrangé" <berrange@redhat.com>]

On Thu, Oct 31, 2024 at 03:09:40PM -0400, Peter Xu wrote:
> On Thu, Oct 31, 2024 at 03:53:47PM +0000, Daniel P. Berrangé wrote:
> > Since object_new_with_class() accepts a non-const parameter for
> > the class, callers should be prepared for failures from unexpected
> > input. Add an Error parameter for this and make callers check.
> > If the caller does not already have an Error parameter, it is
> > satisfactory to use &error_abort if the class parameter choice is
> > not driven by untrusted user input.
> > 
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > ---
> >  accel/accel-user.c               |  3 ++-
> >  include/qom/object.h             |  9 +++++++--
> >  net/net.c                        | 10 ++++++----
> >  qom/object.c                     |  4 ++--
> >  system/vl.c                      |  6 ++++--
> >  target/i386/cpu-apic.c           |  8 +++++++-
> >  target/i386/cpu-sysemu.c         | 11 ++++++++---
> >  target/i386/cpu.c                |  4 ++--
> >  target/s390x/cpu_models_sysemu.c |  7 +++++--
> >  9 files changed, 43 insertions(+), 19 deletions(-)
> > 
> > diff --git a/accel/accel-user.c b/accel/accel-user.c
> > index 22b6a1a1a8..04ba4ab920 100644
> > --- a/accel/accel-user.c
> > +++ b/accel/accel-user.c
> > @@ -18,7 +18,8 @@ AccelState *current_accel(void)
> >          AccelClass *ac = accel_find("tcg");
> >  
> >          g_assert(ac != NULL);
> > -        accel = ACCEL(object_new_with_class(OBJECT_CLASS(ac)));
> > +        accel = ACCEL(object_new_with_class(OBJECT_CLASS(ac),
> > +                                            &error_abort));
> >      }
> >      return accel;
> >  }
> > diff --git a/include/qom/object.h b/include/qom/object.h
> > index 2af9854675..222c60e205 100644
> > --- a/include/qom/object.h
> > +++ b/include/qom/object.h
> > @@ -606,14 +606,19 @@ struct InterfaceClass
> >  /**
> >   * object_new_with_class:
> >   * @klass: The class to instantiate.
> > + * @errp: pointer to be filled with error details on failure
> >   *
> >   * This function will initialize a new object using heap allocated memory.
> >   * The returned object has a reference count of 1, and will be freed when
> >   * the last reference is dropped.
> >   *
> > - * Returns: The newly allocated and instantiated object.
> > + * If an instance of @klass is not permitted to be instantiated, an
> > + * error will be raised. This can happen if @klass is abstract.
> > + *
> > + * Returns: The newly allocated and instantiated object, or NULL
> > + * on error.
> >   */
> > -Object *object_new_with_class(ObjectClass *klass);
> > +Object *object_new_with_class(ObjectClass *klass, Error **errp);
> >  
> >  /**
> >   * object_new:
> > diff --git a/net/net.c b/net/net.c
> > index d9b23a8f8c..7fb5e966f3 100644
> > --- a/net/net.c
> > +++ b/net/net.c
> > @@ -944,11 +944,13 @@ GPtrArray *qemu_get_nic_models(const char *device_type)
> >               * create this property during instance_init, so we have to create
> >               * a temporary instance here to be able to check it.
> >               */
> > -            Object *obj = object_new_with_class(OBJECT_CLASS(dc));
> > -            if (object_property_find(obj, "netdev")) {
> > -                g_ptr_array_add(nic_models, (gpointer)name);
> > +            Object *obj = object_new_with_class(OBJECT_CLASS(dc), NULL);
> 
> One trivial comment: I kind of understand why NULL was chosen, but I don't
> think it's easily understandable on why it's better.
> 
> When object_new() can have side effect and might fail, logically it could
> be better that it asserts failure here when new NICs added (that can start
> to fail it here.. while we shouldn't have such now), instead of silently
> not showing up in the module list.  So at least we notify the net developer
> something might be off (while IIUC this function is trying to list all NIC
> modules QEMU supports).

This change is a mistake. This commit ought to be keeping failure
behaviour broadly the same, just pushing error handling up a level.
So from that POV, I should have used &error_abort here.

Any relaxation of error handling to be more graceful here ought
to be a separately justified commit.


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|