On Tue, Apr 23, 2013 at 03:08:23PM -0600, Eric Blake wrote:
> On 04/23/2013 09:57 AM, Daniel P. Berrange wrote:
> > 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.
We could probably just list all source files, since the
script will just skip over their content unless it finds
something named 'vir*Driver'. The downside would be making
the script slower to run due to redundant I/O. Probably
worth it on balance though. I'll do a followup.
> > +
> > +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.
Heh, you can tell I always tested by running 'make check-driverimpls' :-)
> > +++ 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...)
I thought about this quite alot. In the end I decided not to
touch this. I know we prefer this style, but we have been
absolutely awful at doing it consistently. IMHO if we want
to keep this is a rule, then we need todo something serious
to enforce it at make check time. Relying on our reviewers
is just not proving reliable enough.
I'm wondering about perhaps running 'indent | diff' during
make check to validate whether any code gets re-formatted
by changes.
This would of course require us to do a big one-time reformat
to get into an initial consistent state.
I've also not been able to figure out how to make indent
respect our preferred style for function parameters. ie
all params on one line, but if line wrapping is needed,
then put each param on a separate line, aligned with (.
> > @@ -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.
Yeah, this is slightly awful, but I didn't want to mix in
semantic changes. Basically all methods are doing non-graceful
shutdown in OpenVZ.
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list