[asterisk-dev] [Code Review] A name/value list mechanism for raising events.

[Mark Michelson]

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



/trunk/include/asterisk/nvevent.h
<https://reviewboard.asterisk.org/r/2248/#comment14392>

    You could instead require a buffer and size argument so that you can just copy the cached string into the buffer. As of now, you're giving back a field of an ast_nv_field directly, which means that if the ast_nv_field were freed, you'd have a pointer to invalid memory.



/trunk/include/asterisk/nvevent.h
<https://reviewboard.asterisk.org/r/2248/#comment14391>

    This function claims that if no ID is provided, that a UUID will be created for you. However, this does not appear to happen.



/trunk/include/asterisk/nvevent.h
<https://reviewboard.asterisk.org/r/2248/#comment14393>

    These lines have red blobs at the beginning. Maybe you used spaces instead of tabs?



/trunk/main/nvevent.c
<https://reviewboard.asterisk.org/r/2248/#comment14390>

    The id field of an ast_nv_object() is never set. I assume you  meant to do it in ast_nv_object_create() based on the passed in ID.



/trunk/main/nvevent.c
<https://reviewboard.asterisk.org/r/2248/#comment14389>

    Actually, you could do this if it's better than having to potentially realloc several times or allocate more space than is required.
    
    You can make use of the va_copy macro in order to create a copy of the varargs that were passed in. On one copy, iterate over them all to figure out how many there are, then allocate the appropriate number of fields, then use the other copy to actually fill in the field data.
    
    Though, to be honest, I have no idea if this is actually any better or worse than what currently exists here.



/trunk/main/nvevent.c
<https://reviewboard.asterisk.org/r/2248/#comment14394>

    Check for allocation failure.



/trunk/tests/test_nvevent.c
<https://reviewboard.asterisk.org/r/2248/#comment14388>

    Expand this debug statement to state what the expected and actual field names are.



/trunk/tests/test_nvevent.c
<https://reviewboard.asterisk.org/r/2248/#comment14395>

    Red blob here.


- Mark


On Dec. 20, 2012, 8:36 a.m., Brent Eagles wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/2248/
> -----------------------------------------------------------
> 
> (Updated Dec. 20, 2012, 8:36 a.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> This patch includes an initial crack at a mechanism for creating events through collections of named/value pairs. It also contains some non-heavily-tested code for publishing the name/value pairs as an AMI event. 
> 
> 
> Diffs
> -----
> 
>   /trunk/include/asterisk/nvevent.h PRE-CREATION 
>   /trunk/main/nvevent.c PRE-CREATION 
>   /trunk/tests/test_nvevent.c PRE-CREATION 
> 
> Diff: https://reviewboard.asterisk.org/r/2248/diff
> 
> 
> Testing
> -------
> 
> Unit test in test_nvevent.c
> 
> 
> Thanks,
> 
> Brent
> 
>

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