On 04/23/2013 09:57 AM, Daniel P. Berrange wrote:
> From: "Daniel P. Berrange" <berrange@xxxxxxxxxx>
>
> 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@xxxxxxxxxx>
> ---
> 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
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list