[libvirt] [PATCH 5/6] libvirt: Ensure modern APIs are implemented

[Daniel P. Berrangé]

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 at redhat.com>
> > ---
> >  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 :|