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

[Mark Michelson]

> On Aug. 1, 2013, 4:35 p.m., Mark Michelson wrote:
> > /trunk/main/bucket.c, lines 165-168
> > <https://reviewboard.asterisk.org/r/2715/diff/1/?file=43192#file43192line165>
> >
> >     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)
> 
> Joshua Colp wrote:
>     Unfortunately the way you mention would cause the schemes to never be unloaded, since their reference count would always be 1. I may just err on that side regardless since unloading/loading schemes should so frequently occur.

An alternative would be to keep the name of the module on the scheme and then retrieve a reference to the module from the loader when you need to call into it. Unfortunately, I don't think the loader provides an API call to retrieve a module by name, and this would also add overhead to the operations.


> On Aug. 1, 2013, 4:35 p.m., Mark Michelson wrote:
> > /trunk/main/bucket.c, lines 360-361
> > <https://reviewboard.asterisk.org/r/2715/diff/1/?file=43192#file43192line360>
> >
> >     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().
> 
> Joshua Colp wrote:
>     Files are not shared, so they need no locking.

Why can't files be shared? As long as an ast_bucket_file_retrieve() function exists, it's possible for multiple locations to share the same resource. This may not be the intended use, but it's possible.


- Mark


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


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