29.08.2013 11:36, Gao feng kirjoitti:
On 08/29/2013 04:20 PM, Oskari Saarenmaa wrote:
On Thu, Aug 29, 2013 at 10:10:36AM +0200, Michal Privoznik wrote:
On 28.08.2013 23:05, Oskari Saarenmaa wrote:
Interface names do not have to be numerical (or veth + number) and trying to assign them to that format is susceptible to race conditions. Instead, assign the parent interface name according to the mac address (the last three bytes) if no name was given by the caller and use the parent interface name plus 'p' for the container name. I don't think it is a good idea. What it user defines MAC addresses in a way that the last three bytes are the same? E.g.
00:11:22:33:44:55 00:22:11:33:44:55
(there are plenty of possibilities). With your code we would create veth334455 for the first domain and there's nothing left for the other one but eyes to cry.
Sure, it's possible for the user to assign addresses like this, but I believe most mac addresses are assigned randomly, that's what libvirt does by default, no? Also, it's possible for the user to override the interface name if they want to. Currently it's only possible to set the first interface name, but there's no way to set the name of the second interface which causes failures because of races. In any case it makes sense to set the second interface's name based on the first interface's name so we don't have to try to come up with two different unique names.
Moreover, there's no race now, as we use global lock to mutually exclude call to virNetDevVethCreate. Although, I must admit it's only within a single driver, I think. So if there are two domains starting (one in qemu driver the other one in lxc, for instance) there can be race. But this should be solved in a different way then. The virNetDevVethCreate should be turned into virObjectLockable and a veth should be allocated by calling a method.
I ran into this issue repeatedly when I tried to start multiple LXC domains simultaneously from my application (https://github.com/melor/poni) which runs libvirt operations in their own threads.
It's easy to be resolved. give the parentVeth and containerVeth a name by default.
At the moment virLXCProcessSetupInterfaceBridged always passes a NULL for the container interface name and while it would be possible to make it configurable like the parent interface name, I believe it makes more sense to just do the right thing by default. The suggested patch also greatly simplifies name selection by removing the loops trying to find a supposedly unused interface name. If you don't like using mac address in the interface name we could just replace it with a random string with a loop checking that the selected name wasn't in use when it was selected. The current approach of trying to use a name with a low number just doesn't work in an environment where another process does the same thing. / Oskari