subject:"Re\: \[libvirt\] \[PATCH\] Account for defined networks when generating bridge names"

Re: [libvirt] [PATCH] Account for defined networks when generating bridge names

2009-03-02 Thread Jim Meyering

Cole Robinson wrote:
...
 Okay, I rolled these changes and the BridgeInUse changes into one patch
 (along with Jim's suggestions).

 I added a helper function SetBridgeName which basically does:

 if user passed bridge name:
   verify it doesn't collide
 else:
   generate bridge name

 We call this in Define and CreateXML. When loading configs from a driver
 restart, we only attempt to generate a bridge: if checking for
 collisions failed, the network wouldn't even be defined, which would
 only cause more pain for users.
...

Hi Cole,

Here's a quick review:
One nit below, but what I'd really like is a stand-alone virsh-oriented
way to exercise a few of these new code paths (tests), just to make sure
we get coverage when running make check.  If you can outline something
like that, I'll massage it into a new test.

 Generate network bridge names if none passed at define/create time.
...
 diff --git a/src/network_conf.c b/src/network_conf.c
 index 6ad0d01..5de164e 100644
 --- a/src/network_conf.c
 +++ b/src/network_conf.c
 @@ -43,6 +43,7 @@
  #include buf.h
  #include c-ctype.h

 +#define MAX_BRIDGE_ID 256
...
 +char *virNetworkAllocateBridge(virConnectPtr conn,
 +   const virNetworkObjListPtr nets)
 +{
 +
 +int id = 0;
 +char *newname;
 +
 +do {
 +char try[50];
 +
 +snprintf(try, sizeof(try), virbr%d, id);
 +
 +if (!virNetworkBridgeInUse(nets, try, NULL)) {
 +if (!(newname = strdup(try))) {
 +virReportOOMError(conn);
 +return NULL;
 +}
 +return newname;
 +}
 +
 +id++;
 +} while (id  MAX_BRIDGE_ID);

This should probably be = MAX_BRIDGE_ID,
or change MAX_BRIDGE_ID to 255, ..
so that the diagnostic below makes sense.

 +virNetworkReportError(conn, VIR_ERR_INTERNAL_ERROR,
 +  _(Bridge generation exceeded max id %d),
 +  MAX_BRIDGE_ID);
 +return NULL;
 +}

--
Libvir-list mailing list
Libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] Account for defined networks when generating bridge names

2009-03-02 Thread Daniel P. Berrange

On Fri, Feb 27, 2009 at 10:57:35AM -0500, Cole Robinson wrote:
 Daniel P. Berrange wrote:
  On Mon, Feb 16, 2009 at 06:40:59PM -0500, Cole Robinson wrote:
  diff --git a/src/network_driver.c b/src/network_driver.c
  index d750565..d83f902 100644
  --- a/src/network_driver.c
  +++ b/src/network_driver.c
  @@ -812,7 +812,12 @@ static int networkStartNetworkDaemon(virConnectPtr 
  conn,
   return -1;
   }
   
  -if ((err = brAddBridge(driver-brctl, network-def-bridge))) {
  +if (!network-def-bridge 
  +!(network-def-bridge = virNetworkAllocateBridge(conn,
  +  
  driver-networks)))
  +return -1;
  +
  +if ((err = brAddBridge(driver-brctl, network-def-bridge))) {
  
  This will cause a thread deadlock once you add the locking I describe
  for virNetworkBridgeInUse() in the previous patch. This is because
  the current virNetworkObjPtr 'network'  here will be locked, then
  the function you're calling with then try to lock it again. 
  
  A deep called function like networkStartNetworkDaemon() shouldn't be
  iterating over all network objects, so this is the wrong place to try
  and fix this problem.
  
  I'm guessing you're trying to fix up existing defined networks without
  a bridge here, so IMHO, this is better done at daemon startup, where
  we load all the configs off disk. This will avoid the locking trouble
  
  Do it in 'networkStartup',m just after the virNetworkLoadAllConfigs
  call, but before autostart is done.
  
 
 Okay, I rolled these changes and the BridgeInUse changes into one patch
 (along with Jim's suggestions).
 
 I added a helper function SetBridgeName which basically does:
 
 if user passed bridge name:
   verify it doesn't collide
 else:
   generate bridge name
 
 We call this in Define and CreateXML. When loading configs from a driver
 restart, we only attempt to generate a bridge: if checking for
 collisions failed, the network wouldn't even be defined, which would
 only cause more pain for users.

ACK, this looks safe now.

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@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] Account for defined networks when generating bridge names

2009-03-02 Thread Cole Robinson

Jim Meyering wrote:
 Cole Robinson wrote:
 ...
 Okay, I rolled these changes and the BridgeInUse changes into one patch
 (along with Jim's suggestions).

 I added a helper function SetBridgeName which basically does:

 if user passed bridge name:
   verify it doesn't collide
 else:
   generate bridge name

 We call this in Define and CreateXML. When loading configs from a driver
 restart, we only attempt to generate a bridge: if checking for
 collisions failed, the network wouldn't even be defined, which would
 only cause more pain for users.
 ...
 
 Hi Cole,
 
 Here's a quick review:
 One nit below, but what I'd really like is a stand-alone virsh-oriented
 way to exercise a few of these new code paths (tests), just to make sure
 we get coverage when running make check.  If you can outline something
 like that, I'll massage it into a new test.

There's a few pieces:

Define a virtual network without a 'bridge=' attribute, then do
something like 'virsh net-dumpxml netname | grep virbr'. Previously
nothing would be shown, now it should match the generated bridge name.

Define a virtual network with an already in use bridge (simplest to use
would be virbr0 since the 'default' network uses it.) Now it will
rightfully error, previously no complaints. The same for 'virsh create'
(though this would fail previously if the colliding network was running,
but with an unclear error message).

Unfortunately none of this will work for the test driver, since all
these changes were specific to the actual network driver. It would have
been nice to do the collision detections in the xml parsing routines but
I think it breaks the API separation too much.


 
 Generate network bridge names if none passed at define/create time.
 ...
 diff --git a/src/network_conf.c b/src/network_conf.c
 index 6ad0d01..5de164e 100644
 --- a/src/network_conf.c
 +++ b/src/network_conf.c
 @@ -43,6 +43,7 @@
  #include buf.h
  #include c-ctype.h

 +#define MAX_BRIDGE_ID 256
 ...
 +char *virNetworkAllocateBridge(virConnectPtr conn,
 +   const virNetworkObjListPtr nets)
 +{
 +
 +int id = 0;
 +char *newname;
 +
 +do {
 +char try[50];
 +
 +snprintf(try, sizeof(try), virbr%d, id);
 +
 +if (!virNetworkBridgeInUse(nets, try, NULL)) {
 +if (!(newname = strdup(try))) {
 +virReportOOMError(conn);
 +return NULL;
 +}
 +return newname;
 +}
 +
 +id++;
 +} while (id  MAX_BRIDGE_ID);
 
 This should probably be = MAX_BRIDGE_ID,
 or change MAX_BRIDGE_ID to 255, ..
 so that the diagnostic below makes sense.
 

Ahh! I forgot about this change and already committed the patch. I'll
commit this as an incremental fix. Thanks for the review.

- Cole

--
Libvir-list mailing list
Libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] Account for defined networks when generating bridge names

2009-02-27 Thread Cole Robinson

Daniel P. Berrange wrote:
 On Mon, Feb 16, 2009 at 06:40:59PM -0500, Cole Robinson wrote:
 diff --git a/src/network_driver.c b/src/network_driver.c
 index d750565..d83f902 100644
 --- a/src/network_driver.c
 +++ b/src/network_driver.c
 @@ -812,7 +812,12 @@ static int networkStartNetworkDaemon(virConnectPtr conn,
  return -1;
  }
  
 -if ((err = brAddBridge(driver-brctl, network-def-bridge))) {
 +if (!network-def-bridge 
 +!(network-def-bridge = virNetworkAllocateBridge(conn,
 +  
 driver-networks)))
 +return -1;
 +
 +if ((err = brAddBridge(driver-brctl, network-def-bridge))) {
 
 This will cause a thread deadlock once you add the locking I describe
 for virNetworkBridgeInUse() in the previous patch. This is because
 the current virNetworkObjPtr 'network'  here will be locked, then
 the function you're calling with then try to lock it again. 
 
 A deep called function like networkStartNetworkDaemon() shouldn't be
 iterating over all network objects, so this is the wrong place to try
 and fix this problem.
 
 I'm guessing you're trying to fix up existing defined networks without
 a bridge here, so IMHO, this is better done at daemon startup, where
 we load all the configs off disk. This will avoid the locking trouble
 
 Do it in 'networkStartup',m just after the virNetworkLoadAllConfigs
 call, but before autostart is done.
 

Okay, I rolled these changes and the BridgeInUse changes into one patch
(along with Jim's suggestions).

I added a helper function SetBridgeName which basically does:

if user passed bridge name:
  verify it doesn't collide
else:
  generate bridge name

We call this in Define and CreateXML. When loading configs from a driver
restart, we only attempt to generate a bridge: if checking for
collisions failed, the network wouldn't even be defined, which would
only cause more pain for users.

Patch below.

Thanks,
Cole


Generate network bridge names if none passed at define/create time.

diff --git a/src/bridge.c b/src/bridge.c
index 668dcf0..46dc407 100644
--- a/src/bridge.c
+++ b/src/bridge.c
@@ -49,7 +49,7 @@
 #include util.h
 #include logging.h
 
-#define MAX_BRIDGE_ID 256
+#define MAX_TAP_ID 256
 
 #define JIFFIES_TO_MS(j) (((j)*1000)/HZ)
 #define MS_TO_JIFFIES(ms) (((ms)*HZ)/1000)
@@ -127,32 +127,13 @@ brShutdown(brControl *ctl)
 #ifdef SIOCBRADDBR
 int
 brAddBridge(brControl *ctl,
-char **name)
+const char *name)
 {
 if (!ctl || !ctl-fd || !name)
 return EINVAL;
 
-if (*name) {
-if (ioctl(ctl-fd, SIOCBRADDBR, *name) == 0)
-return 0;
-} else {
-int id = 0;
-do {
-char try[50];
-
-snprintf(try, sizeof(try), virbr%d, id);
-
-if (ioctl(ctl-fd, SIOCBRADDBR, try) == 0) {
-if (!(*name = strdup(try))) {
-ioctl(ctl-fd, SIOCBRDELBR, name);
-return ENOMEM;
-}
-return 0;
-}
-
-id++;
-} while (id  MAX_BRIDGE_ID);
-}
+if (ioctl(ctl-fd, SIOCBRADDBR, name) == 0)
+return 0;
 
 return errno;
 }
@@ -547,7 +528,7 @@ brAddTap(brControl *ctl,
 }
 
 id++;
-} while (subst  id = MAX_BRIDGE_ID);
+} while (subst  id = MAX_TAP_ID);
 
  error:
 close(fd);
diff --git a/src/bridge.h b/src/bridge.h
index f37ab72..e06ff41 100644
--- a/src/bridge.h
+++ b/src/bridge.h
@@ -47,7 +47,7 @@ int brInit  (brControl **ctl);
 voidbrShutdown  (brControl *ctl);
 
 int brAddBridge (brControl *ctl,
- char **name);
+ const char *name);
 int brDeleteBridge  (brControl *ctl,
  const char *name);
 int brHasBridge (brControl *ctl,
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index c38e142..b8cca00 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -206,6 +206,7 @@ virNetworkObjListFree;
 virNetworkDefParseNode;
 virNetworkRemoveInactive;
 virNetworkSaveConfig;
+virNetworkSetBridgeName;
 virNetworkObjLock;
 virNetworkObjUnlock;
 
diff --git a/src/network_conf.c b/src/network_conf.c
index 6ad0d01..5de164e 100644
--- a/src/network_conf.c
+++ b/src/network_conf.c
@@ -43,6 +43,7 @@
 #include buf.h
 #include c-ctype.h
 
+#define MAX_BRIDGE_ID 256
 #define VIR_FROM_THIS VIR_FROM_NETWORK
 
 VIR_ENUM_DECL(virNetworkForward)
@@ -743,6 +744,12 @@ virNetworkObjPtr virNetworkLoadConfig(virConnectPtr conn,
 goto error;
 }
 
+/* Generate a bridge if none is found, but don't check for collisions
+ * if a bridge is hardcoded, so the network is at least defined
+ */
+if (!def-bridge  !(def-bridge = virNetworkAllocateBridge(conn, nets)))
+goto error;
+
 if (!(net = virNetworkAssignDef(conn, nets, def)))
 goto error;

Re: [libvirt] [PATCH] Account for defined networks when generating bridge names

2009-02-18 Thread Jim Meyering

Cole Robinson crobi...@redhat.com wrote:
 This is the second part of the fix for rhbz 479622. If we are generating
 a bridge name for a virtual network, don't collide with any bridge name
 in a defined network. This patch also generates a bridge name at network
 define time, if none was passed in the xml.

 The downside to all this is that it won't fix things for existing
 victims of the bug: if they have 2 networks with the same bridge device
 in the xml, we can't intelligently remedy the situation. This patch just
 helps prevent future users getting into that predicament.

 Thanks,
 Cole

 diff --git a/src/bridge.c b/src/bridge.c
 index fc11429..4446a95 100644
 --- a/src/bridge.c
 +++ b/src/bridge.c
 @@ -49,7 +49,7 @@
  #include util.h
  #include logging.h

 -#define MAX_BRIDGE_ID 256
 +#define MAX_TAP_ID 256

  #define JIFFIES_TO_MS(j) (((j)*1000)/HZ)
  #define MS_TO_JIFFIES(ms) (((ms)*HZ)/1000)
 @@ -127,32 +127,13 @@ brShutdown(brControl *ctl)
  #ifdef SIOCBRADDBR
  int
  brAddBridge(brControl *ctl,
 -char **name)
 +char *name)

Hi Cole,

Presuming this part will stay, as you rework the patch,
please make that const.
and maybe save a line of vertical space:

  brAddBridge(brControl *ctl, const char *name)

--
Libvir-list mailing list
Libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] Account for defined networks when generating bridge names

2009-02-17 Thread Daniel P. Berrange

On Mon, Feb 16, 2009 at 06:40:59PM -0500, Cole Robinson wrote:
 diff --git a/src/network_driver.c b/src/network_driver.c
 index d750565..d83f902 100644
 --- a/src/network_driver.c
 +++ b/src/network_driver.c
 @@ -812,7 +812,12 @@ static int networkStartNetworkDaemon(virConnectPtr conn,
  return -1;
  }
  
 -if ((err = brAddBridge(driver-brctl, network-def-bridge))) {
 +if (!network-def-bridge 
 +!(network-def-bridge = virNetworkAllocateBridge(conn,
 +  
 driver-networks)))
 +return -1;
 +
 +if ((err = brAddBridge(driver-brctl, network-def-bridge))) {

This will cause a thread deadlock once you add the locking I describe
for virNetworkBridgeInUse() in the previous patch. This is because
the current virNetworkObjPtr 'network'  here will be locked, then
the function you're calling with then try to lock it again. 

A deep called function like networkStartNetworkDaemon() shouldn't be
iterating over all network objects, so this is the wrong place to try
and fix this problem.

I'm guessing you're trying to fix up existing defined networks without
a bridge here, so IMHO, this is better done at daemon startup, where
we load all the configs off disk. This will avoid the locking trouble

Do it in 'networkStartup',m just after the virNetworkLoadAllConfigs
call, but before autostart is done.

 @@ -1147,11 +1152,18 @@ static virNetworkPtr networkDefine(virConnectPtr 
 conn, const char *xml) {
  if (!(def = virNetworkDefParseString(conn, xml)))
  goto cleanup;
  
 -if (def-bridge 
 -virNetworkBridgeInUse(driver-networks, def-bridge)) {
 -networkReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR,
 -   _(bridge name '%s' already in use.), 
 def-bridge);
 -goto cleanup;
 +if (def-bridge) {
 +if (virNetworkBridgeInUse(driver-networks, def-bridge)) {
 +networkReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR,
 +   _(bridge name '%s' already in use.),
 +   def-bridge);
 +goto cleanup;
 +}
 +} else {
 +/* Allocate a bridge name */
 +if (!(def-bridge = virNetworkAllocateBridge(conn,
 + driver-networks)))
 +goto cleanup;
  }

This bit is OK from a locking POV, since its at the top level entry
point.

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@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] Account for defined networks when generating bridge names

Re: [libvirt] [PATCH] Account for defined networks when generating bridge names

Re: [libvirt] [PATCH] Account for defined networks when generating bridge names

Re: [libvirt] [PATCH] Account for defined networks when generating bridge names

Re: [libvirt] [PATCH] Account for defined networks when generating bridge names

Re: [libvirt] [PATCH] Account for defined networks when generating bridge names

6 matches

Site Navigation

Mail list logo

Footer information