Re: [PATCH v3 1/9] hw: eliminate qdev_try_new, isa_try_new & usb_try_new

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

On Tue, Dec 03, 2024 at 04:30:06PM +0100, Markus Armbruster wrote:
> Daniel P. Berrangé <berrange@redhat.com> writes:
> 
> > These functions all return NULL rather than asserting, if the requested
> > type is not registered and also cannot be dynamically loaded.
> >
> > In several cases their usage is pointless, since the caller then just
> > reports an error & exits anyway. Easier to just let qdev_new fail
> > with &error_fatal.
> 
> Uh, this sounds as if you'd turn assertion failures by fatal errors,
> which could be fine, but more than just "eliminate qdev_try_new...".
> 
> Turns out you aren't.  qdev_new(), isa_new() and usb_new() are all thin
> wrappers around object_new(), which does not assert, but treats errors
> as fatal:
> 
>     Object *object_new(const char *typename)
>     {
>         TypeImpl *ti = type_get_or_load_by_name(typename, &error_fatal);
> 
>         return object_new_with_type(ti);
>     }
> 
> type_get_or_load_by_name() succeeds if
> 
> * type @typename is compiled into this binary, or
> 
> * exactly one module providing it is known to this binary, and loading
>   it succeeds.
> 
> Put a pin into this for later.
> 
> Suggest something like
> 
>   The difference between qdev_try_new() and qdev_try() is that the
>   former returns failure, while the latter treats it as fatal and
>   terminates the process.  Same for isa_try_new() and usb_try_new().
> 
> A comment in hw/i2c/i2c.h mentions i2c_slave_try_new(), but it doesn't
> exist, and never has.  Suggest to eliminate that, too.
> 
> > In other cases, the desired semantics are clearer to understand if the
> > caller directly checks module_object_class_by_name(), before calling
> > the regular qdev_new (or specialized equiv) function.
> 
> This tacitly assumes qdev_try_new() & friends fail exactly when
> module_object_class_by_name() fails.  True, but not obvious at this
> point.
> 
> It's true, because it also fails exactly when type_get_or_load_by_name()
> returns null:
> 
>     ObjectClass *object_class_by_name(const char *typename)
>     {
>         TypeImpl *type = type_get_by_name_noload(typename);
> 
>         if (!type) {
>             return NULL;
>         }
> 
>         type_initialize(type);
> 
>         return type->class;
>     }
> 


> > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > index f9147fecbd..d668970bee 100644
> > --- a/hw/i386/pc.c
> > +++ b/hw/i386/pc.c
> > @@ -596,9 +596,11 @@ static gboolean pc_init_ne2k_isa(ISABus *bus, NICInfo *nd, Error **errp)
> >                     "maximum number of ISA NE2000 devices exceeded");
> >          return false;
> >      }
> > -    isa_ne2000_init(bus, ne2000_io[nb_ne2k],
> > -                    ne2000_irq[nb_ne2k], nd);
> > -    nb_ne2k++;
> > +    if (module_object_class_by_name(TYPE_ISA_NE2000)) {
> > +        isa_ne2000_init(bus, ne2000_io[nb_ne2k],
> > +                        ne2000_irq[nb_ne2k], nd);
> > +        nb_ne2k++;
> > +    }
> 
> This gave me pause until I saw the change to isa_ne2000_init() below.
> There, you replace isa_try_new() by isa_new().  Before the patch,
> isa_ne2000_init() can fail, afterwards it treats errors as fatal.  And
> that's why you need to guard against failure here.
> 
> In other words, you lifted the guard out of isa_ne2000_init() into its
> sole caller.  Fine, just less than obvious in review.

Yeah, actually this is a pre-existing bug I should fix in a
preceeding patch.  isa_ne2000_init can fail today, but we
don't check the return value, and unconditionally do
"nb_ne2k++". So nb_ne2k is wrong if isa_ne2000_init ever
fails. Not sure if this has any bad functional effect,
but conceptually it is clearly a bug.

> 
> >      return true;
> >  }
> >  
> > @@ -1087,7 +1089,7 @@ static void pc_superio_init(ISABus *isa_bus, bool create_fdctrl,
> >      int i;
> >      DriveInfo *fd[MAX_FD];
> >      qemu_irq *a20_line;
> > -    ISADevice *i8042, *port92, *vmmouse;
> > +    ISADevice *i8042, *port92, *vmmouse = NULL;
> >  
> >      serial_hds_isa_init(isa_bus, 0, MAX_ISA_SERIAL_PORTS);
> >      parallel_hds_isa_init(isa_bus, MAX_PARALLEL_PORTS);
> > @@ -1117,9 +1119,9 @@ static void pc_superio_init(ISABus *isa_bus, bool create_fdctrl,
> >      i8042 = isa_create_simple(isa_bus, TYPE_I8042);
> >      if (!no_vmport) {
> >          isa_create_simple(isa_bus, TYPE_VMPORT);
> > -        vmmouse = isa_try_new("vmmouse");
> > -    } else {
> > -        vmmouse = NULL;
> > +        if (module_object_class_by_name("vmmouse")) {
> > +            vmmouse = isa_new("vmmouse");
> > +        }
> >      }
> >      if (vmmouse) {
> >          object_property_set_link(OBJECT(vmmouse), TYPE_I8042, OBJECT(i8042),
> 
> This is now like
> 
>        vmmouse = NULL;
>        if (...) {
>            if (module_object_class_by_name("vmmouse")) {
>                vmmouse = isa_new("vmmouse");
>            }
>        }
>        if (vmmouse) {
>            object_property_set_link(OBJECT(vmmouse), TYPE_I8042, OBJECT(i8042),
>                                     &error_abort);
>            isa_realize_and_unref(vmmouse, isa_bus, &error_fatal);
>        }
> 
> We could straighten control flow like this:
> 
>        if (...) {
>            if (module_object_class_by_name("vmmouse")) {
>                vmmouse = isa_new("vmmouse");
>                object_property_set_link(OBJECT(vmmouse), TYPE_I8042,
>                                         OBJECT(i8042), &error_abort);
>                isa_realize_and_unref(vmmouse, isa_bus, &error_fatal);
>            }
>        }
> 
> But there is a more fundamental issue.
> 
> pc_superio_init() creates onboard devices.
> 
> With CONFIG_MODULES off, it creates a "vmmouse" device exactly when the
> type is compiled into this binary.  This makes the guest machine type
> depend on build configuration.  I consider this questionable; I'd prefer
> such things to be explicit in the C code.  But let's ignore this.

Yeah, I had the same horrified realization that we'd made machine ABI
vary based on installed pkgs :-( Not sure how to get us out of that
mess easily.

> Silently not creating it just because the machine is in a funny state,
> say temporarily lacks the resources to load a DSO, is plainly wrong.
> 
> Not this patch's fault.  Doesn't make it less wrong :)

Agreed, we definitely need to distinguish "module not installed",
from all other types of failure to load a module.

> > @@ -1163,11 +1165,7 @@ void pc_basic_device_init(struct PCMachineState *pcms,
> >      if (pcms->hpet_enabled) {
> >          qemu_irq rtc_irq;
> >  
> > -        hpet = qdev_try_new(TYPE_HPET);
> > -        if (!hpet) {
> > -            error_report("couldn't create HPET device");
> > -            exit(1);
> > -        }
> > +        hpet = qdev_new(TYPE_HPET);
> 
> This replaces the error message "couldn't create HPET device" by one
> provided by QOM.  These are:
> 
> * When the type is not known to this binary: "unknown type 'hpet'".
> 
> * When the type is known, but not compiled in, and the module can't be
>   loaded for whatever reason: "could not load a module for type 'hpet':
>   MORE", where MORE is the error message provided by module_load_qom().
> 
> Worth at least hinting at this in the commit message?

Sure.




> > diff --git a/include/hw/usb.h b/include/hw/usb.h
> > index d46d96779a..bb778cb844 100644
> > --- a/include/hw/usb.h
> > +++ b/include/hw/usb.h
> > @@ -584,11 +584,6 @@ static inline USBDevice *usb_new(const char *name)
> >      return USB_DEVICE(qdev_new(name));
> >  }
> >  
> > -static inline USBDevice *usb_try_new(const char *name)
> > -{
> > -    return USB_DEVICE(qdev_try_new(name));
> > -}
> > -
> >  static inline bool usb_realize_and_unref(USBDevice *dev, USBBus *bus, Error **errp)
> >  {
> >      return qdev_realize_and_unref(&dev->qdev, &bus->qbus, errp);
> 
> Maybe I'm having another scatter-brained day, but I found the patch
> somewhat confusing in review.  Happy to suggest a possible split if
> you're interested.

I can have another think about changing it. Mostly I was just working
backwards when creating this, by deleting the methods I wanted to
remove and the patching up the build failures, so there wasn't much
thought put into the split of this one.

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 :|