On Tue, Jul 09, 2019 at 11:02:03AM +0200, Peter Krempa wrote:
> On Mon, Jul 08, 2019 at 22:37:02 -0500, Eric Blake wrote:
> > As shown in recent patches, several drivers provided only an older
> > counterpart of an API, making it harder to uniformly use the newer
> > preferred API form. We can prevent future instances of this by failing
> > the driver at initialization time if a modern API is forgotten when an
> > older API is present. For now, the list includes any interface with a
> > Flags counterpart, except virDomainBlockStatsFlags which is a bit more
> > complex than virDomainBlockStats.
> >
> > Signed-off-by: Eric Blake <eblake@xxxxxxxxxx>
> > ---
> > src/libvirt.c | 42 ++++++++++++++++++++++++++++++++++++++----
> > 1 file changed, 38 insertions(+), 4 deletions(-)
> >
> > diff --git a/src/libvirt.c b/src/libvirt.c
> > index 7e665b6cba..a12a72a31b 100644
> > --- a/src/libvirt.c
> > +++ b/src/libvirt.c
> > @@ -567,19 +567,53 @@ int
> > virRegisterConnectDriver(virConnectDriverPtr driver,
> > bool setSharedDrivers)
> > {
> > - VIR_DEBUG("driver=%p name=%s", driver,
> > - driver ? NULLSTR(driver->hypervisorDriver->name) : "(null)");
> > + const char *driver_name;
> > +
> > + driver_name = driver ? NULLSTR(driver->hypervisorDriver->name) : "(null)";
> > + VIR_DEBUG("driver=%p name=%s", driver, driver_name);
> >
> > virCheckNonNullArgReturn(driver, -1);
> > if (virConnectDriverTabCount >= MAX_DRIVERS) {
> > virReportError(VIR_ERR_INTERNAL_ERROR,
> > _("Too many drivers, cannot register %s"),
> > - driver->hypervisorDriver->name);
> > + driver_name);
> > return -1;
> > }
> >
> > + /* Check for drivers failing to provide a modern counterpart to an
> > + * older API */
> > +#define REQUIRE_API(old, new) \
> > + do { \
> > + if (driver->hypervisorDriver->old && \
> > + !driver->hypervisorDriver->new) { \
> > + fprintf(stderr, " ***FIXME!: driver %s is broken on %s\n", \
> > + driver ? NULLSTR(driver->hypervisorDriver->name) : "(null)", #new); \
>
> fprintf in a library function is really wrong.
>
> Also I don't think this really requires a runtime check as the APIs
> aren't really going to just disappear.
Yeah, I think we can easily do this validation via a make check rule
using static analysis.
We could hack either check-driverimpls.pl or check-drivername.pl to
mandate that both the old + new method are always provided as a pair,
or just create a new dedicated check script for that.
Failing during "make check" is preferable to printf at runtime.
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 :|
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
