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

Wed Nov 28 15:31:37 CST 2012

> 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.

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.

> On Nov. 28, 2012, 11:53 a.m., David Lee wrote:
> > /trunk/main/uuid.c, line 102
> > <https://reviewboard.asterisk.org/r/2217/diff/2/?file=32250#file32250line102>
> >
> >     The concerns about a random generator would be 1) performance and 2) thread safety. You may want to address those here, too.

See my comment below. Once that gets resolved, I can be sure to add comments to the source.

> 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.

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?

> On Nov. 28, 2012, 11:53 a.m., David Lee wrote:
> > /trunk/include/asterisk/uuid.h, line 50
> > <https://reviewboard.asterisk.org/r/2217/diff/2/?file=32247#file32247line50>
> >
> >     Since UUID strings are a fixed size (37 chars, including null terminator), I would be fine with a bare char* here.
> >     
> >     Either way, you should define AST_UUID_STRLEN so callers can properly size their strings before calling.

See my comment at the bottom. If we change the requirements of the UUID API to only work with RFC 4122 UUIDs, then I can instead make this take a buffer and size. I can also change the function to return the buffer so that people can do things like:

char buf[AST_UUID_STRLEN];
ast_debug(1, "Object is at address %p and its UUID is %s\n", obj, ast_uuid_to_str(obj->uuid, buf, sizeof(buf));

> On Nov. 28, 2012, 11:53 a.m., David Lee wrote:
> > /trunk/include/asterisk/uuid.h, line 62
> > <https://reviewboard.asterisk.org/r/2217/diff/2/?file=32247#file32247line62>
> >
> >     This should at least take a char*. It seems more straightforward to do that than force callers to wrap their pointers in ast_str.

I can agree with this, especially if I'm going to change the ast_uuid_to_str(). I used an ast_str here to complement ast_uuid_to_str().

- Mark

-----------------------------------------------------------
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/ebf5d657/attachment-0001.htm>