Re: [libvirt] [PATCH] Account for defined networks when generating bridge names
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