[PATCH] network: Introduce mutex for bridge name generation

Michal Privoznik posted 1 patch 1 week, 6 days ago
Test syntax-check failed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/96002b1b5d178fe4ae3de2ecd200cd2b96e61e49.1610032153.git.mprivozn@redhat.com
src/network/bridge_driver.c | 13 +++++++++++--
1 file changed, 11 insertions(+), 2 deletions(-)

[PATCH] network: Introduce mutex for bridge name generation

Posted by Michal Privoznik 1 week, 6 days ago
When defining/creating a network the bridge name may be filled in
automatically by libvirt (if none provided in the input XML or
the one provided is a pattern, e.g. "virbr%d"). During the
bridge name generation process a candidate name is generated
which is then checked with the rest of already defined/running
networks for collisions.

Problem is, that there is no mutex guarding this critical section
and thus if two threads line up so that they both generate the
same candidate they won't find any collision and the same name is
then stored.

Closes: https://gitlab.com/libvirt/libvirt/-/issues/78
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
 src/network/bridge_driver.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index a7c5aade14..b7c604eaea 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -74,6 +74,8 @@
 #define VIR_FROM_THIS VIR_FROM_NETWORK
 #define MAX_BRIDGE_ID 256
 
+static virMutex bridgeNameValidateMutex = VIR_MUTEX_INITIALIZER;
+
 /**
  * VIR_NETWORK_DHCP_LEASE_FILE_SIZE_MAX:
  *
@@ -3115,20 +3117,27 @@ static int
 networkBridgeNameValidate(virNetworkObjListPtr nets,
                           virNetworkDefPtr def)
 {
+    virMutexLock(&bridgeNameValidateMutex);
+
     if (def->bridge && !strstr(def->bridge, "%d")) {
         if (virNetworkObjBridgeInUse(nets, def->bridge, def->name)) {
             virReportError(VIR_ERR_INTERNAL_ERROR,
                            _("bridge name '%s' already in use."),
                            def->bridge);
-            return -1;
+            goto error;
         }
     } else {
         /* Allocate a bridge name */
         if (networkFindUnusedBridgeName(nets, def) < 0)
-            return -1;
+            goto error;
     }
 
+    virMutexUnlock(&bridgeNameValidateMutex);
     return 0;
+
+ error:
+    virMutexUnlock(&bridgeNameValidateMutex);
+    return -1;
 }
 
 
-- 
2.26.2

Re: [PATCH] network: Introduce mutex for bridge name generation

Posted by Laine Stump 1 week, 6 days ago
On 1/7/21 10:09 AM, Michal Privoznik wrote:
> When defining/creating a network the bridge name may be filled in
> automatically by libvirt (if none provided in the input XML or
> the one provided is a pattern, e.g. "virbr%d"). During the
> bridge name generation process a candidate name is generated
> which is then checked with the rest of already defined/running
> networks for collisions.
>
> Problem is, that there is no mutex guarding this critical section
> and thus if two threads line up so that they both generate the
> same candidate they won't find any collision and the same name is
> then stored.
>
> Closes: https://gitlab.com/libvirt/libvirt/-/issues/78


"Closes:"? I'm guessing other people have also been using this tag to 
get gitlab to automatically close PRs and I just haven't noticed it 
until now, but according to this page:

https://docs.gitlab.com/ee/user/project/issues/managing_issues.html#closing-issues

"Resolves:" also works, and is a tag that has already been used quite a 
bit in libvirt in the past.


On the other hand, I've had some people tell me that they want just the 
URL of the issue that was fixed, with no explicit tag (although that was 
for bugzilla bugs)


Is it worth trying to pick one of these to always use, or is that just 
pointless micromanagement? Or maybe there was already a discussion and I 
just missed it... (I'm undecided whether I lean towards OCD, or "Freedum!!")


> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>


Reviewed-by: Laine Stump <laine@redhat.com>


Re: [PATCH] network: Introduce mutex for bridge name generation

Posted by Ján Tomko 1 week, 6 days ago
On a Thursday in 2021, Laine Stump wrote:
>On 1/7/21 10:09 AM, Michal Privoznik wrote:
>>When defining/creating a network the bridge name may be filled in
>>automatically by libvirt (if none provided in the input XML or
>>the one provided is a pattern, e.g. "virbr%d"). During the
>>bridge name generation process a candidate name is generated
>>which is then checked with the rest of already defined/running
>>networks for collisions.
>>
>>Problem is, that there is no mutex guarding this critical section
>>and thus if two threads line up so that they both generate the
>>same candidate they won't find any collision and the same name is
>>then stored.
>>
>>Closes: https://gitlab.com/libvirt/libvirt/-/issues/78
>
>
>"Closes:"? I'm guessing other people have also been using this tag to 
>get gitlab to automatically close PRs and I just haven't noticed it 
>until now, but according to this page:
>
>https://docs.gitlab.com/ee/user/project/issues/managing_issues.html#closing-issues
>
>"Resolves:" also works, and is a tag that has already been used quite 
>a bit in libvirt in the past.
>

Even for GitLab issues, Resolves is slightly winning at 7 vs 5.

>
>On the other hand, I've had some people tell me that they want just 
>the URL of the issue that was fixed, with no explicit tag (although 
>that was for bugzilla bugs)
>

Yes, I considered it nicer and less deceitful (because you're not really
claiming anything just by including the link), back then when I cared about things.

>
>Is it worth trying to pick one of these to always use, or is that just 
>pointless micromanagement?

Of course, that's what a mailing list is for.

>Or maybe there was already a discussion and 
>I just missed it... (I'm undecided whether I lean towards OCD, or 
>"Freedum!!")
>

As long as both people and machines can read it, either is fine.

Jano

>
>>Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>
>
>Reviewed-by: Laine Stump <laine@redhat.com>
>
>

Re: [PATCH] network: Introduce mutex for bridge name generation

Posted by Daniel Henrique Barboza 1 week, 6 days ago

On 1/7/21 5:22 PM, Ján Tomko wrote:
> On a Thursday in 2021, Laine Stump wrote:
>> On 1/7/21 10:09 AM, Michal Privoznik wrote:
>>> When defining/creating a network the bridge name may be filled in
>>> automatically by libvirt (if none provided in the input XML or
>>> the one provided is a pattern, e.g. "virbr%d"). During the
>>> bridge name generation process a candidate name is generated
>>> which is then checked with the rest of already defined/running
>>> networks for collisions.
>>>
>>> Problem is, that there is no mutex guarding this critical section
>>> and thus if two threads line up so that they both generate the
>>> same candidate they won't find any collision and the same name is
>>> then stored.
>>>
>>> Closes: https://gitlab.com/libvirt/libvirt/-/issues/78
>>
>>
>> "Closes:"? I'm guessing other people have also been using this tag to get gitlab to automatically close PRs and I just haven't noticed it until now, but according to this page:
>>
>> https://docs.gitlab.com/ee/user/project/issues/managing_issues.html#closing-issues
>>
>> "Resolves:" also works, and is a tag that has already been used quite a bit in libvirt in the past.
>>
> 
> Even for GitLab issues, Resolves is slightly winning at 7 vs 5.


I'm using 'Resolves: <gitlab bug link>' because I saw someone else doing
it. I thought that the reasoning behind it is that 'Resolves' is when you
want to say that 'this fixes the following bug entry', which differs from
'Fixes', which is used generally in the format 'Fixes: <commit>' to indicate
that it's an amend of another commit.

> 
>>
>> On the other hand, I've had some people tell me that they want just the URL of the issue that was fixed, with no explicit tag (although that was for bugzilla bugs)


If we can make it a standard, like, every time a bug link is posted in the end
of a commit message means 'this patch fixes this bug', then sure, why not.



>>
> 
> Yes, I considered it nicer and less deceitful (because you're not really
> claiming anything just by including the link), back then when I cared about things.


It's a good time to stop caring too much. We're barely a week in 2021 and stuff is
already weirder than before.

> 
>>
>> Is it worth trying to pick one of these to always use, or is that just pointless micromanagement?
> 
> Of course, that's what a mailing list is for.

Deep down, I'm replying to this because I'm expecting Laine to tell some
good story dowm the road and I want to be in the CC.



Thanks


DHB

> 
>> Or maybe there was already a discussion and I just missed it... (I'm undecided whether I lean towards OCD, or "Freedum!!")
>>
> 
> As long as both people and machines can read it, either is fine.
> 
> Jano
> 
>>
>>> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>>
>>
>> Reviewed-by: Laine Stump <laine@redhat.com>
>>
>>

Re: [PATCH] network: Introduce mutex for bridge name generation

Posted by Erik Skultety 1 week, 6 days ago
On Thu, Jan 07, 2021 at 07:33:38PM -0300, Daniel Henrique Barboza wrote:
> 
> 
> On 1/7/21 5:22 PM, Ján Tomko wrote:
> > On a Thursday in 2021, Laine Stump wrote:
> > > On 1/7/21 10:09 AM, Michal Privoznik wrote:
> > > > When defining/creating a network the bridge name may be filled in
> > > > automatically by libvirt (if none provided in the input XML or
> > > > the one provided is a pattern, e.g. "virbr%d"). During the
> > > > bridge name generation process a candidate name is generated
> > > > which is then checked with the rest of already defined/running
> > > > networks for collisions.
> > > > 
> > > > Problem is, that there is no mutex guarding this critical section
> > > > and thus if two threads line up so that they both generate the
> > > > same candidate they won't find any collision and the same name is
> > > > then stored.
> > > > 
> > > > Closes: https://gitlab.com/libvirt/libvirt/-/issues/78
> > > 
> > > 
> > > "Closes:"? I'm guessing other people have also been using this tag to get gitlab to automatically close PRs and I just haven't noticed it until now, but according to this page:
> > > 
> > > https://docs.gitlab.com/ee/user/project/issues/managing_issues.html#closing-issues
> > > 
> > > "Resolves:" also works, and is a tag that has already been used quite a bit in libvirt in the past.
> > > 
> > 
> > Even for GitLab issues, Resolves is slightly winning at 7 vs 5.
> 
> 
> I'm using 'Resolves: <gitlab bug link>' because I saw someone else doing
> it. I thought that the reasoning behind it is that 'Resolves' is when you
> want to say that 'this fixes the following bug entry', which differs from
> 'Fixes', which is used generally in the format 'Fixes: <commit>' to indicate
> that it's an amend of another commit.

Oops, I just used Fixes: with an issue link and gitlab happily ate it as well
and closed the issue.

Erik

Re: [PATCH] network: Introduce mutex for bridge name generation

Posted by Daniel P. Berrangé 1 week, 6 days ago
On Fri, Jan 08, 2021 at 08:28:28AM +0100, Erik Skultety wrote:
> On Thu, Jan 07, 2021 at 07:33:38PM -0300, Daniel Henrique Barboza wrote:
> > 
> > 
> > On 1/7/21 5:22 PM, Ján Tomko wrote:
> > > On a Thursday in 2021, Laine Stump wrote:
> > > > On 1/7/21 10:09 AM, Michal Privoznik wrote:
> > > > > When defining/creating a network the bridge name may be filled in
> > > > > automatically by libvirt (if none provided in the input XML or
> > > > > the one provided is a pattern, e.g. "virbr%d"). During the
> > > > > bridge name generation process a candidate name is generated
> > > > > which is then checked with the rest of already defined/running
> > > > > networks for collisions.
> > > > > 
> > > > > Problem is, that there is no mutex guarding this critical section
> > > > > and thus if two threads line up so that they both generate the
> > > > > same candidate they won't find any collision and the same name is
> > > > > then stored.
> > > > > 
> > > > > Closes: https://gitlab.com/libvirt/libvirt/-/issues/78
> > > > 
> > > > 
> > > > "Closes:"? I'm guessing other people have also been using this tag to get gitlab to automatically close PRs and I just haven't noticed it until now, but according to this page:
> > > > 
> > > > https://docs.gitlab.com/ee/user/project/issues/managing_issues.html#closing-issues
> > > > 
> > > > "Resolves:" also works, and is a tag that has already been used quite a bit in libvirt in the past.
> > > > 
> > > 
> > > Even for GitLab issues, Resolves is slightly winning at 7 vs 5.
> > 
> > 
> > I'm using 'Resolves: <gitlab bug link>' because I saw someone else doing
> > it. I thought that the reasoning behind it is that 'Resolves' is when you
> > want to say that 'this fixes the following bug entry', which differs from
> > 'Fixes', which is used generally in the format 'Fixes: <commit>' to indicate
> > that it's an amend of another commit.
> 
> Oops, I just used Fixes: with an issue link and gitlab happily ate it as well
> and closed the issue.

It accepts essentially anything that people might commonly use:

https://docs.gitlab.com/ee/user/project/issues/managing_issues.html#default-closing-pattern


 * Close, Closes, Closed, Closing, close, closes, closed, closing
 * Fix, Fixes, Fixed, Fixing, fix, fixes, fixed, fixing
 * Resolve, Resolves, Resolved, Resolving, resolve, resolves, resolved, resolving
 * Implement, Implements, Implemented, Implementing, implement, implements, implemented, implementing 

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|