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é <berra...@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 :|
