On Wed, Jan 14, 2009 at 08:55:46AM +0000, Mark McLoughlin wrote:
> On Tue, 2009-01-13 at 20:49 +0000, Daniel P. Berrange wrote:
> > Currently when we shutdown the virtual networks are all shutdown too.
> > + errno = EINVAL;
> > + return -1;
> > + }
> > +
> > + memset(&ifr, 0, sizeof(struct ifreq));
> > +
> > + strncpy(ifr.ifr_name, name, len);
> > + ifr.ifr_name[len] = '\0';
> > +
> > + if (ioctl(ctl->fd, SIOCGIFMTU, &ifr))
> > + return -1;
>
> I'd probably use SIOCGIFFLAGS for this.
Yeah, sounds good to me - I just picked the first side-effect
free thing that I found. SIOCGIFFLAGS would let us check that
the interface was actually up, as well as merely existing.
> >
> > +int virNetworkSaveConfig(virConnectPtr conn,
> > + const char *configDir,
> > + virNetworkDefPtr def)
> > +{
> > + int ret = -1;
> > + char *xml;
> > +
> > + if (!(xml = virNetworkDefFormat(conn,
> > + def)))
>
> Odd formatting.
Cut & paste from the virDomainSaveConfig, which had a number
of extra args to this equivalent method, hence multi-line.
> > diff --git a/src/network_driver.c b/src/network_driver.c
> > --- a/src/network_driver.c
> > +++ b/src/network_driver.c
> > @@ -57,6 +57,8 @@
> > #include "iptables.h"
> > #include "bridge.h"
> >
> > +#define NETWORK_PID_DIR LOCAL_STATE_DIR "/run/libvirt/network"
> > +#define NETWORK_LIB_DIR LOCAL_STATE_DIR "/lib/libvirt/network"
>
> s/LIB_DIR/STATE_DIR/ perhaps?
Yes, would make more sense.
>
> Also, I'd prefer to see dirs like this created by "make install" and/or
> RPM install.
>
> e.g. with read-only root if you wanted /var/lib/libvirt to be read-only
> and bind-mount a tmpfs onto /var/lib/libvirt/network
Will sort that. SHould also pre-create the directories here used
by LXC / UML / QEMU drivers.
> > @@ -377,15 +443,6 @@ networkBuildDnsmasqArgv(virConnectPtr co
> > APPEND_ARG(*argv, i++, "--except-interface");
> > APPEND_ARG(*argv, i++, "lo");
> >
> > - /*
> > - * NB, dnsmasq command line arg bug means we need to
> > - * use a single arg '--dhcp-leasefile=path' rather than
> > - * two separate args in '--dhcp-leasefile path' style
> > - */
>
> Worth keeping this comment - e.g. someone could come along and change it
> do s/=/ /, check that it works with newer dnsmasq and then 6 months
> later get a report that it doesn't work with older dnsmasq.
>
> > - snprintf(buf, sizeof(buf), "--dhcp-leasefile=%s/lib/libvirt/dhcp-%s.leases",
> > - LOCAL_STATE_DIR, network->def->name);
> > - APPEND_ARG(*argv, i++, buf);
>
> --dhcp-leasefile not needed? Worth a comment in the commit log.
Turns out this support has been compiled out of the Fedora RPMs
for years, so its a silent no-op. Upstream has officially deprecated
its use and indicated that it will be removed, and if you've not
compiled out of the binary, its use will throw an error on startup.
It also isn't used for storing lease dnsmasq gives out - its only
for reading in a pre-defined set of mappings at startup. So basically
useless :-)
> > +static int networkShutdownNetworkDaemon(virConnectPtr conn,
> > + struct network_driver *driver,
> > + virNetworkObjPtr network) {
> > int err;
> > + char *configFile;
> >
> > networkLog(NETWORK_INFO, _("Shutting down network '%s'\n"), network->def->name);
> >
> > if (!virNetworkIsActive(network))
> > return 0;
> >
> > + configFile = virNetworkConfigFile(conn, NETWORK_LIB_DIR, network->def->name);
> > + if (!configFile)
> > + return -1;
> > +
> > + unlink(configFile);
> > + VIR_FREE(configFile);
>
> Perhaps rename this to stateFile - I got confused for a second thinking
> you were deleting the actual config file here.
Ha, ok will rename that.
>
> > if (network->dnsmasqPid > 0)
> > kill(network->dnsmasqPid, SIGTERM);
> >
> > @@ -824,13 +926,10 @@ static int networkShutdownNetworkDaemon(
> > network->def->bridge, strerror(err));
> > }
> >
> > + /* See if its still alive and really really kill it */
> > if (network->dnsmasqPid > 0 &&
> > - waitpid(network->dnsmasqPid, NULL, WNOHANG) != network->dnsmasqPid) {
> > + (kill(network->dnsmasqPid, 0) == 0))
> > kill(network->dnsmasqPid, SIGKILL);
> > - if (waitpid(network->dnsmasqPid, NULL, 0) != network->dnsmasqPid)
> > - networkLog(NETWORK_WARN,
> > - "%s", _("Got unexpected pid for dnsmasq\n"));
> > - }
>
> Why no more waitpid? Zombies, no?
I removed the --keep-in-foreground flag from dnsmasq args, so the momnet
we start it, it daemonizes itself. We launch with virRun() instead of
virExec() which does the neccessary waitpid() at startup, so when shutting
it down, there's nothing to wait for.
Regards,
Daniel
--
|: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list