Hi,
Le vendredi 21 septembre 2012 à 16:28 -0400, Laine Stump a écrit :
> On 09/21/2012 02:45 PM, Eric Blake wrote:
> > From: Benjamin Cama <benjamin.cama@xxxxxxxxxxxxxxxxxxx>
> >
> > Le mercredi 20 juin 2012 à 11:05 +0100, Daniel P. Berrange a écrit :
> >> I think you also need to have a VIR_FOFRCE_CLOSE(tapfd) in the
> >> very last of the cleanup jump labels. Since between the time
> >> we open the tapfd & close it, we might have done a 'goto err1'
> >> or similar.
> > Yes, I forgot that. New patch following.
> >
> >> Looks reasonable apart from the cleanup bug.
> > Thanks. BTW, if this patch is commited, I'm already in AUTHORS under
> > another email address, which is OK.
> > ---
> >
> > This is my attempt at resolving the merge conflict, but I'm not
> > confident enough with the original patch, nor with my test
> > environment, to see if I actually solved anything (not to mention
> > the commit message used by 'git am' is horrible). Is it worth
> > using this version?
Thanks Eric for your attempt, this is also what I came too. I will add
Laine's comment suggestion, though.
> Whatever we push should have the commit log message from the first
> version of the patch. That gave a very good description of the problem,
> and why the patch is IFUPing a tap device that's connected to the
> bridge, but has no IP address (v6 or v4), no backend connection, and no
> traffic flowing over it. It seems very counterintuitive until you read
> the explanation in the original version.
I will repost with the original (updated) description.
>
>
> >
> > src/network/bridge_driver.c | 22 ++++++++++++++++++++--
> > src/uml/uml_conf.c | 3 ++-
> > src/util/virnetdevtap.c | 24 ++++++++++++++----------
> > src/util/virnetdevtap.h | 2 ++
> > 4 files changed, 38 insertions(+), 13 deletions(-)
> >
> > diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
> > index fce1739..33aba74 100644
> > --- a/src/network/bridge_driver.c
> > +++ b/src/network/bridge_driver.c
> > @@ -65,6 +65,7 @@
> > #include "virnetdevtap.h"
> > #include "virnetdevvportprofile.h"
> > #include "virdbus.h"
> > +#include "virfile.h"
> >
> > #define NETWORK_PID_DIR LOCALSTATEDIR "/run/libvirt/network"
> > #define NETWORK_STATE_DIR LOCALSTATEDIR "/lib/libvirt/network"
> > @@ -993,6 +994,7 @@ networkRadvdConfContents(virNetworkObjPtr network, char **configstr)
> > " AdvSendAdvert on;\n"
> > " AdvManagedFlag off;\n"
> > " AdvOtherConfigFlag off;\n"
> > + " IgnoreIfMissing on;\n"
>
> This was added to the patch after V2, and I don't see any explanation
> why. Does this just silence the complaint log messages when the dummy
> tap is down?
No, it allows radvd to start even when the bridge is not RUNNING, as
sometimes radvd binds to it _after_ we set it “down” (by closing the tap
fd). Contrary to what dnsmasq does, radvd daemonizes itself before
binding itself to the interface, so if it comes “too late”, it fails and
stops (and BTW, libvirt doesn't detect that it fails, which may be a
problem in some other cases).
Now that I re-read the code, maybe we could just start radvd before
dnsmasq (it's the contrary currently), as the latter adds a delay that
should be enough for radvd to start and bind itself. But with my patch,
we play on the safe side.
>
>
> > "\n",
> > network->def->bridge);
> >
> > @@ -2061,6 +2063,7 @@ networkStartNetworkVirtual(struct network_driver *driver,
> > virErrorPtr save_err = NULL;
> > virNetworkIpDefPtr ipdef;
> > char *macTapIfName = NULL;
> > + int tapfd = -1;
> >
> > /* Check to see if any network IP collides with an existing route */
> > if (networkCheckRouteCollision(network) < 0)
> > @@ -2082,10 +2085,13 @@ networkStartNetworkVirtual(struct network_driver *driver,
> > virReportOOMError();
> > goto err0;
> > }
> > + /* Keep tun fd open and interface up to allow for IPv6 DAD to happen */
> > if (virNetDevTapCreateInBridgePort(network->def->bridge,
> > &macTapIfName, &network->def->mac,
> > - NULL, NULL, NULL, NULL,
> > - VIR_NETDEV_TAP_CREATE_USE_MAC_FOR_BRIDGE) < 0) {
> > + NULL, &tapfd, NULL, NULL,
> > + VIR_NETDEV_TAP_CREATE_USE_MAC_FOR_BRIDGE |
> > + VIR_NETDEV_TAP_CREATE_IFUP |
> > + VIR_NETDEV_TAP_CREATE_PERSIST) < 0) {
> > VIR_FREE(macTapIfName);
> > goto err0;
> > }
> > @@ -2149,6 +2155,16 @@ networkStartNetworkVirtual(struct network_driver *driver,
> > if (v6present && networkStartRadvd(network) < 0)
> > goto err4;
> >
> > + /* DAD has happened (dnsmasq waits for it),
>
> Hmm. dnsmasq waits for it because it wants to listen for DNS packets on
> the bridge's IPv6 address, right? Okay, so the timing problem mentioned
> in the earlier messages is a thing of the past.
I am not sure if you are talking about the radvd timing issue; if this
is it, see my above remark.
>
>
> > dnsmasq is now bound to the
> > + * bridge's IPv6 address, and radvd to the interface, so we can now set the
> > + * dummy tun down.
> > + */
> > + if (tapfd >= 0) {
> > + if (virNetDevSetOnline(macTapIfName, false) < 0)
> > + goto err4;
> > + VIR_FORCE_CLOSE(tapfd);
> > + }
> > +
> > if (virNetDevBandwidthSet(network->def->bridge, network->def->bandwidth) < 0) {
> > virReportError(VIR_ERR_INTERNAL_ERROR,
> > _("cannot set bandwidth limits on %s"),
> > @@ -2187,6 +2203,8 @@ networkStartNetworkVirtual(struct network_driver *driver,
> > save_err = virSaveLastError();
> >
> > if (macTapIfName) {
> > + if (tapfd >= 0)
> > + VIR_FORCE_CLOSE(tapfd);
> > ignore_value(virNetDevTapDelete(macTapIfName));
> > VIR_FREE(macTapIfName);
> > }
> > diff --git a/src/uml/uml_conf.c b/src/uml/uml_conf.c
> > index a317bcc..e8b8779 100644
> > --- a/src/uml/uml_conf.c
> > +++ b/src/uml/uml_conf.c
> > @@ -142,7 +142,8 @@ umlConnectTapDevice(virConnectPtr conn,
> > vm->uuid, NULL,
> > virDomainNetGetActualVirtPortProfile(net),
> > virDomainNetGetActualVlan(net),
> > - VIR_NETDEV_TAP_CREATE_IFUP) < 0) {
> > + VIR_NETDEV_TAP_CREATE_IFUP |
> > + VIR_NETDEV_TAP_CREATE_PERSIST) < 0) {
> > if (template_ifname)
> > VIR_FREE(net->ifname);
> > goto error;
> > diff --git a/src/util/virnetdevtap.c b/src/util/virnetdevtap.c
> > index 9f2dca1..0eadd6c 100644
> > --- a/src/util/virnetdevtap.c
> > +++ b/src/util/virnetdevtap.c
> > @@ -112,18 +112,20 @@ virNetDevProbeVnetHdr(int tapfd)
> > *
> > * VIR_NETDEV_TAP_CREATE_VNET_HDR
> > * - Enable IFF_VNET_HDR on the tap device
> > + * VIR_NETDEV_TAP_CREATE_PERSIST
> > + * - The device will persist after the file descriptor is closed
> > *
> > * Creates a tap interface.
> > - * If the @tapfd parameter is supplied, the open tap device file
> > - * descriptor will be returned, otherwise the TAP device will be made
> > - * persistent and closed. The caller must use virNetDevTapDelete to
> > - * remove a persistent TAP devices when it is no longer needed.
> > + * If the @tapfd parameter is supplied, the open tap device file descriptor
> > + * will be returned, otherwise the TAP device will be closed. The caller must
> > + * use virNetDevTapDelete to remove a persistent TAP device when it is no
> > + * longer needed.
> > *
> > * Returns 0 in case of success or -1 on failure.
> > */
> > int virNetDevTapCreate(char **ifname,
> > int *tapfd,
> > - unsigned int flags ATTRIBUTE_UNUSED)
> > + unsigned int flags)
> > {
> > int fd;
> > struct ifreq ifr;
> > @@ -160,7 +162,7 @@ int virNetDevTapCreate(char **ifname,
> > goto cleanup;
> > }
> >
> > - if (!tapfd &&
> > + if ((flags & VIR_NETDEV_TAP_CREATE_PERSIST) &&
> > (errno = ioctl(fd, TUNSETPERSIST, 1))) {
> > virReportSystemError(errno,
> > _("Unable to set tap device %s to persistent"),
> > @@ -261,14 +263,16 @@ int virNetDevTapDelete(const char *ifname ATTRIBUTE_UNUSED)
> > * - Enable IFF_VNET_HDR on the tap device
> > * VIR_NETDEV_TAP_CREATE_USE_MAC_FOR_BRIDGE
> > * - Set this interface's MAC as the bridge's MAC address
> > + * VIR_NETDEV_TAP_CREATE_PERSIST
> > + * - The device will persist after the file descriptor is closed
> > *
> > * This function creates a new tap device on a bridge. @ifname can be either
> > * a fixed name or a name template with '%d' for dynamic name allocation.
> > * in either case the final name for the bridge will be stored in @ifname.
> > - * If the @tapfd parameter is supplied, the open tap device file
> > - * descriptor will be returned, otherwise the TAP device will be made
> > - * persistent and closed. The caller must use virNetDevTapDelete to remove
> > - * a persistent TAP devices when it is no longer needed.
> > + * If the @tapfd parameter is supplied, the open tap device file descriptor
> > + * will be returned, otherwise the TAP device will be closed. The caller must
> > + * use virNetDevTapDelete to remove a persistent TAP device when it is no
> > + * longer needed.
> > *
> > * Returns 0 in case of success or -1 on failure
> > */
> > diff --git a/src/util/virnetdevtap.h b/src/util/virnetdevtap.h
> > index fa6a647..980db61 100644
> > --- a/src/util/virnetdevtap.h
> > +++ b/src/util/virnetdevtap.h
> > @@ -43,6 +43,8 @@ typedef enum {
> > VIR_NETDEV_TAP_CREATE_VNET_HDR = 1 << 1,
> > /* Set this interface's MAC as the bridge's MAC address */
> > VIR_NETDEV_TAP_CREATE_USE_MAC_FOR_BRIDGE = 1 << 2,
> > + /* The device will persist after the file descriptor is closed */
> > + VIR_NETDEV_TAP_CREATE_PERSIST = 1 << 3,
> > } virNetDevTapCreateFlags;
> >
> > int virNetDevTapCreateInBridgePort(const char *brname,
>
> This has my ACK as long as:
>
> 1) the original commit message (with some modifications) is used,
>
> 2) the reason for adding the "IgnoreIfMissing" line is added to that
> commit message (and maybe as a comment in the code).
I will do both.
Patch coming shortly after this message. I have tested it here (even if
it's getting harder and harder to get it compiled & packaged for
squeeze…) and it works OK.
Regards,
--
Benjamin Cama <benjamin.cama@xxxxxxxxxxxxxxxxxxx>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list