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

Thu Aug 1 11:35:49 CDT 2013

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


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

    Looking at the implementation of ast_bucket_retrieve(), it appears the documentation here is incorrect. The docs say to pass in a full URI for the bucket. The actual implementation performs ast_sorcery_retrieve_by_id(). The sorcery ID for buckets is the URI name, not the full URI.


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

    I think the URI name parsing done here needs to be improved. If given something like "eggs://foo/bar" then the name will be set to "bar". This ends up being the bucket's sorcery ID.
    
    Later, let's say you create a bucket called "bacon://baz/bar". Now you have two buckets with the same sorcery ID of "bar", even though they have distinct URIs (and distinct URI schemes, for that matter).
    
    I think that the URI name really has little utility within the bucket API. The only thing the bucket API should do when given a URI name is ensure that it is not zero-length.
    
    I think the full URI should be the sorcery ID for buckets and bucket files, and I think you may have had that intention based on the documentation of ast_bucket_retrieve().


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

    This module refcounting model doesn't make much sense. Your scheme keeps a bare pointer to the module. You then bump the refcount of the module before actually calling into it and then unreffing the module immediately after. There's nothing that prevents the module pointer in the scheme from becoming invalid if, say, the module gets unloaded between when the scheme is allocated and the module refcount is bumped.
    
    I think the module refcount should be bumped on creation of the scheme and then dropped on destruction of the scheme. That way, there is no chance that the module pointer can become invalid.
    
    (Repeat this same issue for all places where the module refcount is used in this way)


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

    I suggest RAII_VAR for this variable.


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

    Suggestion: You can make this replacement operation atomic by either:
    
    1) Allocating the container with the AO2_CONTAINER_ALLOC_OPT_DUPS_REPLACE flag. Note that this would require the container to be sorted.
    
    2) Hold the lock on the file->metadata while performing the ao2_find() and ao2_link(). Pass the OBJ_NOLOCK flag to ao2_find() and ao2_link_options().


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

    I suggest RAII_VAR for this variable.


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

    This only gets the seconds from the timeval. The ast_asprintf call should be something like
    
    ast_asprintf(buf, "%lf", field->tv_sec + field->tv_usec/1000000.0)
    
    or
    
    ast_asprintf(buf, "%lu.%06lu", field->tv_sec, field->tv_us)
    

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

    red blobitty blob


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

    For all the tests where you have this, I recommend initially setting dummy NULL and then calling bucket_test_scheme_register after the initial switch statement.


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

    Change this to be less of a garden path sentence.


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

    You've lost me on this test. Can you explain how it works? Why is "test://tmp/bob" a valid bucket to retrieve? When was this bucket created?
    
    Also, bucket sorcery IDs are the name portion of the URI, not the entire URI. So why is ast_bucket_retrieve(), when given a full URI, expected to be able to retrieve a bucket?


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

    You should probably check that the copy's metadata is what you expect it to be (same goes for the move test).


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

    My comments for bucket_retrieve apply here as well.


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

    Check that the metadata retrieved has the value you expect.


- Mark Michelson


On July 30, 2013, 6: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, 6: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/86d616dd/attachment-0001.htm>