[libvirt] [PATCH 8/6] Make driver method names consistent with public APIs

[Eric Blake]

On 04/23/2013 09:57 AM, Daniel P. Berrange wrote:
> From: "Daniel P. Berrange" <berrange at redhat.com>
> 
> Ensure that all drivers implementing public APIs use a
> naming convention for their implementation that matches
> the public API name.
> 
> eg for the public API   virDomainCreate make sure QEMU
> uses qemuDomainCreate and not qemuDomainStart

Yay - makes it MUCH easier to search for an implementation of a given
public API.

> 
> Signed-off-by: Daniel P. Berrange <berrange at redhat.com>
> ---
>  src/Makefile.am                         |  46 ++++
>  src/check-driverimpls.pl                |  64 +++++

Meat of the patch, plus lots of fallout.

>  37 files changed, 1686 insertions(+), 1523 deletions(-)
>  create mode 100644 src/check-driverimpls.pl

Again, chmod +x on the new .pl file.

> 
> diff --git a/src/Makefile.am b/src/Makefile.am
> index 02fb2ab..1f6a245 100644
> --- a/src/Makefile.am
> +++ b/src/Makefile.am
> @@ -435,6 +435,52 @@ check-drivername:
>  		$(srcdir)/libvirt_qemu.syms \
>  		$(srcdir)/libvirt_lxc.syms
>  
> +DRIVER_SOURCE_FILES = \
> +	esx/esx_device_monitor.c \

Hmm.  This means that every new file has to be added in two locations
(for example, esx_*.c would have to touch DRIVER_SOURCE_FILES and
ESX_DRIVER_SOURCES).  We don't do it very often (usually with the
introduction of new hypervisor drivers or major refactoring), but it
still might be worth thinking if there is any way to minimize the
duplication, by defining DRIVER_SOURCE_FILES in terms of all the
$(*_DRIVER_SOURCES), then having the verification script skip the .h
files in the same list.

But this patch is already big enough that making you send a v2 would
clog even more list traffic, so if you like the idea, I'm okay with
seeing just the interdiff or even doing that in a followup 9/6 patch.

> +
> +check-driverimpls:
> +	$(AM_V_GEN)$(PERL) $(srcdir)/check-driverimpls.pl \
> +		$(DRIVER_SOURCE_FILES)
> +
>  check-local: check-protocol check-symfile check-symsorting \
>  	check-drivername

Oops, you didn't actually wire check-driverimpls into check-local, so
'make check' didn't run it.

>  .PHONY: check-protocol $(PROTOCOL_STRUCTS:structs=struct)
> diff --git a/src/check-driverimpls.pl b/src/check-driverimpls.pl
> new file mode 100644
> index 0000000..2c7f8b1
> --- /dev/null
> +++ b/src/check-driverimpls.pl
> @@ -0,0 +1,64 @@
> +#!/usr/bin/perl
> +
> +use strict;
> +use warnings;

Copyright notice?

> +++ b/src/interface/interface_backend_netcf.c
> @@ -116,9 +116,9 @@ static struct netcf_if *interfaceDriverGetNetcfIF(struct netcf *ncf, virInterfac
>      return iface;
>  }
>  
> -static virDrvOpenStatus interfaceOpenInterface(virConnectPtr conn,
> -                                               virConnectAuthPtr auth ATTRIBUTE_UNUSED,
> -                                               unsigned int flags)
> +static virDrvOpenStatus netcfInterfaceOpen(virConnectPtr conn,
> +                                           virConnectAuthPtr auth ATTRIBUTE_UNUSED,
> +                                           unsigned int flags)

As long as you are reformatting functions, should we clean things up to
consistently do:

static returntype
name(args...)

> @@ -584,7 +584,7 @@ udevIfaceFreeIfaceDef(virInterfaceDef *ifacedef)
>  static int
>  ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3)
>  ATTRIBUTE_NONNULL(4) ATTRIBUTE_RETURN_CHECK
> -udevIfaceGetIfaceDefBond(struct udev *udev,
> +udevGetIfaceDefBond(struct udev *udev,
>                           struct udev_device *dev,
>                           const char *name,
>                           virInterfaceDef *ifacedef)

Looks like you didn't always manage to reindent lines when touching a
multiline declaration.

> +++ b/src/openvz/openvz_driver.c

>  
> +static int
> +openvzDomainDestroy(virDomainPtr dom)
> +{
> +    return openvzDomainShutdownFlags(dom, 0);
> +}
> +
> +static int
> +openvzDomainDestroyFlags(virDomainPtr dom, unsigned int flags)
> +{
> +    return openvzDomainShutdownFlags(dom, flags);
> +}
> +
...
> @@ -2191,9 +2203,9 @@ static virDriver openvzDriver = {
>      .domainShutdown = openvzDomainShutdown, /* 0.3.1 */
>      .domainShutdownFlags = openvzDomainShutdownFlags, /* 0.9.10 */
>      .domainReboot = openvzDomainReboot, /* 0.3.1 */
> -    .domainDestroy = openvzDomainShutdown, /* 0.3.1 */
> -    .domainDestroyFlags = openvzDomainShutdownFlags, /* 0.9.4 */
> -    .domainGetOSType = openvzGetOSType, /* 0.3.1 */
> +    .domainDestroy = openvzDomainDestroy, /* 0.3.1 */
> +    .domainDestroyFlags = openvzDomainDestroyFlags, /* 0.9.4 */

Huh, are shutdown and destroy really the same on OpenVZ?  Since destroy
is guaranteed to work, but shutdown is documented as involving guest
interaction, is this really a bug in this driver?  But your conversion
is sane at preserving existing semantics, even if those semantics are buggy.

> +++ b/src/rpc/gendispatch.pl
> @@ -1426,7 +1426,15 @@ elsif ($mode eq "client") {
>          # print function
>          print "\n";
>          print "static $single_ret_type\n";
> -        print "$structprefix$call->{ProcName}(";
> +        if ($structprefix eq "remote") {
> +            print "$structprefix$call->{ProcName}(";
> +        } else {
> +            my $proc = $call->{ProcName};
> +            my $extra = $structprefix;
> +            $extra =~ s/^(\w)/uc $1/e;
> +            $proc =~ s/^(Domain)(.*)/$1 . $extra . $2/e;
> +            print "remote$proc(";
> +        }
>  
>          print join(", ", @args_list);

Looks okay.

ACK with comments addressed.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 621 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20130423/2ca046b5/attachment-0001.sig>