[asterisk-dev] [Code Review]: Add UUID support to Asterisk

Wed Nov 28 16:17:30 CST 2012

> On Nov. 28, 2012, 11:53 a.m., David Lee wrote:
> > /trunk/main/uuid.c, line 175
> > <https://reviewboard.asterisk.org/r/2217/diff/2/?file=32250#file32250line175>
> >
> >     It would be useful to test for the existing of /dev/urandom and log an error if it doesn't exist.
> >     
> >     /dev/random is a blocking entropy source, and could dramatically affect performance if that's the only generator available.
> >     
> >     If neither are available, libuuid uses rand(), which isn't thread safe.
> 
> Mark Michelson wrote:
>     This comment got me looking into libuuid a bit more.
>     
>     If libuuid uses /dev/random/, it actually sets the reads to be nonblocking. If it fails to read data 16 times in a row, then it will fall back to simply using rand() for the current UUID instead. So performance is not degraded as much as it might be if it blocked while trying to read from /dev/random/
>     
>     I took a look at the libuuid source a bit further, and even when getting data from /dev/random/ or /dev/urandom/, it actually uses the rand() (and possibly some use of jrand48()) function to do some adjustment on the returned bytes. If rand() is not thread-safe, then it would appear that *all* calls to uuid_generate_random() are not thread-safe due to their unprotected calls to rand().
>     It appears that we may need to surround all calls to uuid_generate_random() with a mutex. This may hurt performance some, but given all the locking Asterisk already is doing, I don't think this will be a bottleneck. What do you think?

In practice, if we're using /dev/urandom, this isn't likely to be a problem. libuuid is essentially generating a high quality, 128-bit random number. It then proceeds to XOR that number with a low quality random number generated by rand(). Since XOR preserves entropy, you still have a high quality random number.

This assumes that the thread-unsafe misbehaviors are minor (rand() isn't as random as it should be) and not major (rand() sends a message to SkyNet to start the revolution, posts your bank account information to Twitter, and then formats your hard drive, all the while displaying a laughing, highly pixelated Jolly Roger on the screen).

I dislike the idea of this becoming a contention point in the system, since it will likely be used by several different components. Although, the lock would help in the off chance libuuid falls back to using rand(). Maybe only lock if /dev/urandom is unavailable. Or maybe that's an optimization we can do if we decide that contention on generating UUID's is causing us trouble.

Sorry, I have no answers for you. Just vague ramblings.

> On Nov. 28, 2012, 11:53 a.m., David Lee wrote:
> > /trunk/tests/test_uuid.c, lines 74-77
> > <https://reviewboard.asterisk.org/r/2217/diff/2/?file=32251#file32251line74>
> >
> >     UUID string format is defined in RFC 4122, section 3. Any UUID library should return a 36 character string.
> 
> Mark Michelson wrote:
>     This touches on an interesting point. I coded this in a way where an ast_uuid is an opaque object that in some way produces unique identifiers. If the underlying implementation uses RFC 4122 UUIDs, then that's fine, but the API is not expecting that to be a requirement. If we want to use a library that defines 40 character or variable-length character strings, then the API would not have to change any, just the implementation.
>     
>     This thought process may be overengineering, really, since UUIDs are generally well-established as being those set forth in RFC 4122. I could always go back and set a requirement on the API that the underlying implementation is expected to use RFC 4122 UUIDs as opposed to just any representation of a unique identifier.

Yeah, that feels like overengineering to me. UUID's are defined by RFC 4122. You can come up with schemes for identifiers that are universally unique that's different from RFC 4122, but that seems very unlikely. And it should be a minor change to the API: change AST_UUID_STRLEN to be the string length for the new algorithm.

It doesn't make sense to code for variable length identifiers: it's not like a longer id will be more unique than a shorter one.

- David

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/2217/#review7461
-----------------------------------------------------------

On Nov. 28, 2012, 10:16 a.m., Mark Michelson wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/2217/
> -----------------------------------------------------------
> 
> (Updated Nov. 28, 2012, 10:16 a.m.)
> 
> 
> Review request for Asterisk Developers, Matt Jordan and David Lee.
> 
> 
> Summary
> -------
> 
> This is the first of many parts of the overall goal of improving Asterisk APIs. This adds a UUID API to Asterisk, eventually to be used as a method of uniquely identifying channels and potentially other objects.
> 
> The implementation here uses libuuid, which is packaged by all major Linux distributions. The API is basically a wrapper around libuuid's API, but it hides all implementation details in case we want to switch out libraries at some point.
> 
> Licensing for libuuid is a bit confusing. On one hand, this online man page (http://linux.die.net/man/3/libuuid) claims that the library is distributed under the LGPL version 2. However, when I downloaded the source for the library on my machine, the code had the 3-clause BSD license printed in the source. In either case, I believe use of external library is license-compatible with Asterisk.
> 
> Regarding specific implementation decisions, I think comments in the code should explain why I chose to do things certain ways. Please let me know if anything should be done differently or if you have ideas for tests beyond what I have already included.
> 
> I have no idea what is up with the changes listed in autoconfig.h.in. Those changes showed up when I ran the bootstrap.sh script after changing configure.ac. 
> 
> 
> This addresses bug ASTERISK-20725.
>     https://issues.asterisk.org/jira/browse/ASTERISK-20725
> 
> 
> Diffs
> -----
> 
>   /trunk/configure UNKNOWN 
>   /trunk/configure.ac 376724 
>   /trunk/include/asterisk/autoconfig.h.in 376724 
>   /trunk/include/asterisk/uuid.h PRE-CREATION 
>   /trunk/main/Makefile 376724 
>   /trunk/main/asterisk.c 376724 
>   /trunk/main/uuid.c PRE-CREATION 
>   /trunk/tests/test_uuid.c PRE-CREATION 
> 
> Diff: https://reviewboard.asterisk.org/r/2217/diff
> 
> 
> Testing
> -------
> 
> I have included a set of unit tests that exercise all the APIs. They pass when run locally.
> 
> 
> Thanks,
> 
> Mark
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20121128/cd17adf7/attachment.htm>