[asterisk-dev] [Code Review] 2715: Bucket: A generic file and directory API

Thu Aug 1 14:16:15 CDT 2013

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

Would buckets apply to something like the sound: resources, where a single sound may have multiple files on the filesystem (varying by language and format)?

/trunk/include/asterisk/bucket.h
<https://reviewboard.asterisk.org/r/2715/#comment18304>

    Extra space.

/trunk/include/asterisk/bucket.h
<https://reviewboard.asterisk.org/r/2715/#comment18299>

    Better comment would be that this is storage for name and value; my first thought was that this was an additional value.

/trunk/include/asterisk/bucket.h
<https://reviewboard.asterisk.org/r/2715/#comment18300>

    Unimplemented.

/trunk/include/asterisk/bucket.h
<https://reviewboard.asterisk.org/r/2715/#comment18307>

    Does that mean the actual copy is deferred until bucket_file_create() is called?

/trunk/include/asterisk/bucket.h
<https://reviewboard.asterisk.org/r/2715/#comment18308>

    This is a bit weird. Normally, rename(2) is an atomic operation, and lots of interesting things require it to be atomic.

    I don't know if those interesting things are relevant to how one would use buckets, but it's worth thinking about from an API perspective.

/trunk/include/asterisk/bucket.h
<https://reviewboard.asterisk.org/r/2715/#comment18309>

    Hopefully it's only undefined if the operation succeeds. Can you clarify the comment?

/trunk/include/asterisk/bucket.h
<https://reviewboard.asterisk.org/r/2715/#comment18310>

    Is there anything, other than metadata, that you can update about a bucket_file?

/trunk/include/asterisk/config_options.h
<https://reviewboard.asterisk.org/r/2715/#comment18313>

    Looks like you're exposing an opaque struct just to get at the args field. That feels wrong.

/trunk/main/bucket.c
<https://reviewboard.asterisk.org/r/2715/#comment18318>

    The double slashes really aren't necessary in a URI. Simply looking for the colon is sufficient. RFC 3986 gives the example URI of "urn:example:animal:ferret:nose".

    If you wanted to be truly pedantic, you could reject invalid schemes. Seems a bit over-the-top to me, though.
      scheme = ALPHA *( ALPHA / DIGIT / "+" / "-" / "." )

/trunk/main/bucket.c
<https://reviewboard.asterisk.org/r/2715/#comment18316>

    A red/black tree might be a good choice here, so the listings will be alphabetized (not to mention a stable order). Might also work better for really large buckets.

/trunk/main/bucket.c
<https://reviewboard.asterisk.org/r/2715/#comment18320>

    object_set() steals the reference, even if the set fails. Get rid of the extra unref().

/trunk/main/bucket.c
<https://reviewboard.asterisk.org/r/2715/#comment18321>

    Check for failures.

/trunk/main/bucket.c
<https://reviewboard.asterisk.org/r/2715/#comment18322>

    If you add buckets to json before the loop, it will be cleaned up for you when json is cleaned up.

/trunk/main/bucket.c
<https://reviewboard.asterisk.org/r/2715/#comment18324>

    Same re: error checking.

/trunk/main/bucket.c
<https://reviewboard.asterisk.org/r/2715/#comment18323>

    Same re: adding before the loop.

/trunk/main/bucket.c
<https://reviewboard.asterisk.org/r/2715/#comment18325>

    New _hash and _cmp templates are recommended.

    https://wiki.asterisk.org/wiki/pages/viewpage.action?pageId=25919686

/trunk/main/bucket.c
<https://reviewboard.asterisk.org/r/2715/#comment18326>

    Wouldn't this delete the file when the bucket_file object is cleaned up? That seems very unexpected.

/trunk/main/bucket.c
<https://reviewboard.asterisk.org/r/2715/#comment18327>

    name is unused in this function.

/trunk/main/bucket.c
<https://reviewboard.asterisk.org/r/2715/#comment18329>

    Wait, a temporary file? What's going on here? I think I'm lost.

/trunk/main/bucket.c
<https://reviewboard.asterisk.org/r/2715/#comment18330>

    Might as well strerror(errno) so we know why.

/trunk/main/bucket.c
<https://reviewboard.asterisk.org/r/2715/#comment18331>

    Same here.

/trunk/main/bucket.c
<https://reviewboard.asterisk.org/r/2715/#comment18332>

    RAII_VAR

/trunk/main/bucket.c
<https://reviewboard.asterisk.org/r/2715/#comment18333>

    Unnecessary unref.

/trunk/main/bucket.c
<https://reviewboard.asterisk.org/r/2715/#comment18334>

    Setting metadata on json before the loop will help with cleanup.

/trunk/main/bucket.c
<https://reviewboard.asterisk.org/r/2715/#comment18335>

    New hash() and cmp() templates; see link above.

/trunk/main/bucket.c
<https://reviewboard.asterisk.org/r/2715/#comment18336>

    FWIW, strftime/strptime could used ISO-8601 format instead of seconds[.useconds], if there's value in the timestamp being human readable. We do that in ast_json_timeval(), for example.

/trunk/main/bucket.c
<https://reviewboard.asterisk.org/r/2715/#comment18337>

    If you register the cleanup first, then you can change all your goto's to return -1.

/trunk/tests/test_bucket.c
<https://reviewboard.asterisk.org/r/2715/#comment18340>

    I think Mark's already hammered on this, but I would expect the id of a bucket to be its URI. It does stand for 'uniform resource identifier'.

/trunk/tests/test_bucket.c
<https://reviewboard.asterisk.org/r/2715/#comment18339>

    I hope this is just for the sake of a simpler test, and not how you normally add buckets/files to a bucket.

/trunk/tests/test_bucket.c
<https://reviewboard.asterisk.org/r/2715/#comment18338>

    ast_json_equal() could make the test much more concise.

    You can build the expected JSON using ast_json_pack(), or ast_json_load_string(), and just compare that to what you've got.

/trunk/tests/test_bucket.c
<https://reviewboard.asterisk.org/r/2715/#comment18341>

    Same comment re: id and URI.

/trunk/tests/test_bucket.c
<https://reviewboard.asterisk.org/r/2715/#comment18342>

    So here's what I don't get: what's with the temporary file?

    For many resources, like sound: or recording:, there's already file(s) on the filesystem. How does the temporary file relate to that.

    For other resources, like http:, you actually would want to have a caching layer to avoid re-fetching the same resource multiple times (especially considering that HTTP has very well defined caching semantics).

    I would think that the backend provider would be the one taking care of pointing you to where on the filesystem a resource is.

/trunk/tests/test_bucket.c
<https://reviewboard.asterisk.org/r/2715/#comment18343>

    Why is the second update a problem?

/trunk/tests/test_bucket.c
<https://reviewboard.asterisk.org/r/2715/#comment18344>

    Same comment for validating the expected JSON as above.

- David Lee

On July 30, 2013, 1:39 p.m., Joshua Colp wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/2715/
> -----------------------------------------------------------
> 
> (Updated July 30, 2013, 1:39 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> Bucket is a generic file and directory API implemented as a thin wrapper over sorcery. It decouples the storage of files from the backend and provides the ability to set metadata on file themselves. What it means to have files in a "bucket" and details about the files themselves is left up to the user of the bucket API. It enforces no restrictions. Identification of things is done using URIs, which include a scheme. Modules implement specific schemes (local, for example) and take care of the implementation specific aspect of it. The ability to get bucket and file information in a JSON format is provided, as is the ability to copy and move files.
> 
> Stuff to focus on:
> 
> 1. Is the API sane and complete?
> 2. Do the unit tests cover everything?
> 3. What do we want to do for URI parsing - I wrote a basic extremely optimistic one
> 
> 
> Diffs
> -----
> 
>   /trunk/include/asterisk/bucket.h PRE-CREATION 
>   /trunk/include/asterisk/config_options.h 395727 
>   /trunk/main/asterisk.c 395727 
>   /trunk/main/bucket.c PRE-CREATION 
>   /trunk/main/config_options.c 395727 
>   /trunk/tests/test_bucket.c PRE-CREATION 
> 
> Diff: https://reviewboard.asterisk.org/r/2715/diff/
> 
> 
> Testing
> -------
> 
> Ran unit tests, confirmed functional. Tweaked this to purposely break and confirmed they broke.
> 
> 
> Thanks,
> 
> Joshua Colp
> 
>

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