bpo-32107 - Improve MAC address calculation and fix test_uuid.py by warsaw · Pull Request #4600 · python/cpython

warsaw · 2017-11-28T00:04:50Z

Here's yet another attempt, but this time without the bitmask check for universally administered MAC addresses. That check fails miserably on the buildbots and Travis, so it's not worth it.

https://bugs.python.org/issue32107

The original check was erroneous in two ways. First, the documentation said that "47 bit will never be set in IEEE 802 addresses obtained from network cards". I think this is referring to the universally vs. locally administered MAC addresses. Network cards from hardware manufacturers will always be universally administered in order to guarantee global uniqueness. But from https://en.wikipedia.org/wiki/MAC_address it says that this flag is the "distinguished by setting the second-least-significant bit of the first octet of the address", where a 0 means universally administered and a 1 means it's locally administered. This works out to the 42nd bit when counting the LSB as bit 1, or 1<<41. The second bug is that the original bitmask value isn't right for either description: % ./python.exe -c "from math import log2; print(log2(0x010000000000))" 40.0 This causes the test to fail on valid MAC addresses. Fix this by improving the comment, with references, and using the correct bitmask.

Also, clean up some return sites.

* Add a helper function to determine whether a MAC address is universally administered or not. * In the various MAC getter functions, keep track of the first locally administered MAC, and if no universally administered MAC is found, return the first local MAC. * In the ultimate fallback _random_getnode(), add a comment for the multicast bit being set, and use a better bitmask * In getnode(), just fall back to _random_getnode() explicitly if no other MAC has been found.

While it is required that the multicast bit (1<<40) should be set in random generated addresses, there is no requirement that it should be cleared in addresses obtained from network cards.

The original check was erroneous in two ways. First, the documentation said that "47 bit will never be set in IEEE 802 addresses obtained from network cards". I think this is referring to the universally vs. locally administered MAC addresses. Network cards from hardware manufacturers will always be universally administered in order to guarantee global uniqueness. But from https://en.wikipedia.org/wiki/MAC_address it says that this flag is the "distinguished by setting the second-least-significant bit of the first octet of the address", where a 0 means universally administered and a 1 means it's locally administered. This works out to the 42nd bit when counting the LSB as bit 1, or 1<<41. The second bug is that the original bitmask value isn't right for either description: % ./python.exe -c "from math import log2; print(log2(0x010000000000))" 40.0 This causes the test to fail on valid MAC addresses. Fix this by improving the comment, with references, and using the correct bitmask.

Also, clean up some return sites.

* Add a helper function to determine whether a MAC address is universally administered or not. * In the various MAC getter functions, keep track of the first locally administered MAC, and if no universally administered MAC is found, return the first local MAC. * In the ultimate fallback _random_getnode(), add a comment for the multicast bit being set, and use a better bitmask * In getnode(), just fall back to _random_getnode() explicitly if no other MAC has been found.

…torchaka/cpython into test_uuid This merges the relevant bits of python#4572 and should be considered instead of that PR.

The original check was erroneous in two ways. First, the documentation said that "47 bit will never be set in IEEE 802 addresses obtained from network cards". I think this is referring to the universally vs. locally administered MAC addresses. Network cards from hardware manufacturers will always be universally administered in order to guarantee global uniqueness. But from https://en.wikipedia.org/wiki/MAC_address it says that this flag is the "distinguished by setting the second-least-significant bit of the first octet of the address", where a 0 means universally administered and a 1 means it's locally administered. This works out to the 42nd bit when counting the LSB as bit 1, or 1<<41. The second bug is that the original bitmask value isn't right for either description: % ./python.exe -c "from math import log2; print(log2(0x010000000000))" 40.0 This causes the test to fail on valid MAC addresses. Fix this by improving the comment, with references, and using the correct bitmask.

Also, clean up some return sites.

* Add a helper function to determine whether a MAC address is universally administered or not. * In the various MAC getter functions, keep track of the first locally administered MAC, and if no universally administered MAC is found, return the first local MAC. * In the ultimate fallback _random_getnode(), add a comment for the multicast bit being set, and use a better bitmask * In getnode(), just fall back to _random_getnode() explicitly if no other MAC has been found.

While it is required that the multicast bit (1<<40) should be set in random generated addresses, there is no requirement that it should be cleared in addresses obtained from network cards.

vstinner · 2017-11-28T09:31:53Z

bpo-32107 - Third attempt at fixing test_uuid.py #4600

Please use a better commit title

Why does this PR have so many commits? Try maybe to rebase it?

vstinner · 2017-11-28T09:41:27Z

+    # RFC 4122, $4.1.6 says "For systems with no IEEE address, a randomly or
+    # pseudo-randomly generated value may be used; see Section 4.5.  The
+    # multicast bit must be set in such addresses, in order that they will
+    # never conflict with addresses obtained from network cards."


I prefer "the multicast bit" rather than "eight bit". Please update also the documentation:
https://docs.python.org/dev/library/uuid.html#uuid.getnode

vstinner · 2017-11-28T09:43:11Z

+# significant, or 1<<41.  We'll skip over any locally administered MAC
+# addresses, as it makes no sense to use those in UUID calculation.
+#
+# See https://en.wikipedia.org/wiki/MAC_address#Universal_vs._local


You must document this change in the doc:
https://docs.python.org/dev/library/uuid.html#uuid.getnode

Add also a ".. versionchanged:: 3.7" describing that it changed in Python 3.7.

vstinner · 2017-11-28T09:44:37Z

+#
+# This bit works out to be the 42nd bit counting from 1 being the least
+# significant, or 1<<41.  We'll skip over any locally administered MAC
+# addresses, as it makes no sense to use those in UUID calculation.


Instead of "it makes no sense", I suggest to explain it differently, something like: "prefer universally administered addresses since UUID1 objects should be unique".

vstinner · 2017-11-28T09:47:28Z

                    pass
    except OSError:
        pass
+    return first_local_mac or None


"or None" seems useless here. Same comment above.

bedevere-bot · 2017-11-28T09:48:16Z

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

vstinner · 2017-11-28T09:49:37Z

    def test_netbios_getnode(self):
        node = self.uuid._netbios_getnode()
-        self.check_node(node, network=True)
+        self.check_node(node)


This change seems like a regression, why removing the following test? Maybe added it back here?
self.assertFalse(node & 0x010000000000, hex)

See the original report. This check never failed before on developer's machines and buildbots, but Barry has a machine on which it is failed. While the initial diagnostic of this failure was wrong, actually there are no guaranties that this check always passed.

warsaw · 2017-11-28T14:38:38Z

On Nov 28, 2017, at 04:31, Victor Stinner ***@***.***> wrote: Why does this PR have so many commits? Try maybe to rebase it?

It doesn’t matter since this branch will be squash merged anyway.

warsaw · 2017-11-28T14:51:28Z

On Nov 28, 2017, at 04:48, Victor Stinner ***@***.***> wrote: def _random_getnode(): - """Get a random node ID, with eighth bit set as suggested by RFC 4122.""" + """Get a random node ID.""" + # RFC 4122, $4.1.6 says "For systems with no IEEE address, a randomly or + # pseudo-randomly generated value may be used; see Section 4.5. The + # multicast bit must be set in such addresses, in order that they will + # never conflict with addresses obtained from network cards." I prefer "the multicast bit" rather than "eight bit". Please update also the documentation: https://docs.python.org/dev/library/uuid.html#uuid.getnode

Look closely though - “eight bit” is in a removed part of the docstring. We don’t go into such detail in other docstrings, so I opted to remove it here and explain in more detail in the comment.

except OSError: pass + return first_local_mac or None "or None" seems useless here. Same comment above.

I think there could be a case where first_local_mac is False but not None. The implementation requires a None to be returned when no suitable MAC is found.

* Update the getnode() documentation. * Add a versionchanged note. * Elaborate a comment.

warsaw · 2017-11-28T14:53:13Z

I have made the requested changes; please review again.

bedevere-bot · 2017-11-28T14:53:15Z

Thanks for making the requested changes!

@serhiy-storchaka, @vstinner: please review the changes made to this pull request.

vstinner · 2017-11-28T20:52:50Z

Look closely though - “eight bit” is in a removed part of the docstring. We don’t go into such detail in other docstrings, so I opted to remove it here and explain in more detail in the comment.

I proposed to replace "eighth bit" with "multicast bit" at:
https://docs.python.org/dev/library/uuid.html#uuid.getnode

I was talking about this documentation, the Doc/ directory, not docstrings.

warsaw · 2017-11-28T20:59:47Z

On Nov 28, 2017, at 15:52, Victor Stinner ***@***.***> wrote: I proposed to replace "eighth bit" with "multicast bit" at: https://docs.python.org/dev/library/uuid.html#uuid.getnode I was talking about this documentation, the Doc/ directory, not docstrings.

Yes, my PR already made that change to the docs.

vstinner

LGTM. I just have a minor suggestion on the NEWS entry.

vstinner · 2017-11-28T21:07:04Z

@@ -0,0 +1,4 @@
+Improve the private ``*_getnode()`` methods for UUID1 such that universally


For the NEWS entry, I suggest to only describe changes on the public API. In this case, the change impacts getnode().

vstinner · 2017-11-28T21:08:42Z

Yes, my PR already made that change to the docs.

Oh yes, now I saw the changes, good!

warsaw · 2017-11-28T21:24:29Z

On Nov 28, 2017, at 16:08, Victor Stinner ***@***.***> wrote: For the NEWS entry, I suggest to only describe changes on the public API. In this case, the change impacts getnode().

Good point, thanks.

vstinner

LGTM.

warsaw and others added 30 commits November 21, 2017 12:00

Return None instead of 0

941b47d

Never return a locally administered MAC address

e529214

Also, clean up some return sites.

A missed return

c056584

Two typos

7fd446e

DEBUGGING TRAVIS - DO NOT MERGE

fb2c2f2

Two tests cannot succeed on Travis-CI

99e86b2

bpo-32107: Remove the check of the multicast bit in test_uuid.

74702e3

While it is required that the multicast bit (1<<40) should be set in random generated addresses, there is no requirement that it should be cleared in addresses obtained from network cards.

Return None instead of 0

d3d0465

Never return a locally administered MAC address

7c17cd8

Also, clean up some return sites.

A missed return

af7929c

Two typos

51ba0b3

DEBUGGING TRAVIS - DO NOT MERGE

909ee79

Two tests cannot succeed on Travis-CI

6acb120

Merge branch 'test_uuid' of github.com:warsaw/cpython into test_uuid

b7d647c

Merge branch 'test-uuid-multicast-bit' of https://github.com/serhiy-s…

26db2f5

…torchaka/cpython into test_uuid This merges the relevant bits of python#4572 and should be considered instead of that PR.

Return None instead of 0

83dc3a3

Never return a locally administered MAC address

fb156f3

Also, clean up some return sites.

A missed return

668f4f6

Two typos

6eecb80

DEBUGGING TRAVIS - DO NOT MERGE

87712f3

Two tests cannot succeed on Travis-CI

740393e

bpo-32107: Remove the check of the multicast bit in test_uuid.

225b063

While it is required that the multicast bit (1<<40) should be set in random generated addresses, there is no requirement that it should be cleared in addresses obtained from network cards.

DEBUGGING TRAVIS - DO NOT MERGE

5b8ca90

Two tests cannot succeed on Travis-CI

2aa4f50

Remove some unnecessary comments

eedce47

warsaw requested review from serhiy-storchaka and vstinner November 28, 2017 00:07

serhiy-storchaka approved these changes Nov 28, 2017

View reviewed changes

vstinner requested changes Nov 28, 2017

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting merge labels Nov 28, 2017

vstinner reviewed Nov 28, 2017

View reviewed changes

warsaw changed the title ~~bpo-32107 - Third attempt at fixing test_uuid.py~~ bpo-32107 - Improve MAC address calculation and fix test_uuid.py Nov 28, 2017

Respond to review

55f2da9

* Update the getnode() documentation. * Add a versionchanged note. * Elaborate a comment.

bedevere-bot added awaiting change review and removed awaiting changes labels Nov 28, 2017

vstinner reviewed Nov 28, 2017

View reviewed changes

A better blurb

db9d0cf

vstinner approved these changes Nov 28, 2017

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting change review labels Nov 28, 2017

warsaw merged commit 23df2d1 into python:master Nov 28, 2017

bedevere-bot removed the awaiting merge label Nov 28, 2017

warsaw deleted the test_uuid_3 branch November 28, 2017 22:26

		@@ -0,0 +1,4 @@
		Improve the private ``*_getnode()`` methods for UUID1 such that universally

Uh oh!

Conversation

warsaw commented Nov 28, 2017 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vstinner commented Nov 28, 2017

Uh oh!

vstinner Nov 28, 2017

Choose a reason for hiding this comment

Uh oh!

vstinner Nov 28, 2017

Choose a reason for hiding this comment

Uh oh!

vstinner Nov 28, 2017

Choose a reason for hiding this comment

Uh oh!

vstinner Nov 28, 2017

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Nov 28, 2017

Uh oh!

vstinner Nov 28, 2017

Choose a reason for hiding this comment

Uh oh!

serhiy-storchaka Nov 28, 2017

Choose a reason for hiding this comment

Uh oh!

warsaw commented Nov 28, 2017 via email

Uh oh!

warsaw commented Nov 28, 2017 via email

Uh oh!

warsaw commented Nov 28, 2017

Uh oh!

bedevere-bot commented Nov 28, 2017

Uh oh!

vstinner commented Nov 28, 2017

Uh oh!

warsaw commented Nov 28, 2017 via email

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

vstinner Nov 28, 2017

Choose a reason for hiding this comment

Uh oh!

vstinner commented Nov 28, 2017

Uh oh!

warsaw commented Nov 28, 2017 via email

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

warsaw commented Nov 28, 2017 •

edited by bedevere-bot

Loading