[vlc-devel] [discussion] To be, or not to be, a directory! [ pf_readdir vs STREAM_IS_DIRECTORY ]

Filip Roséen filip at atch.se
Fri Jul 22 01:42:45 CEST 2016


     * stream_t definition
    struct stream_t
    		*/ ... */
         * Read directory.
         * Callback to fill an item node from a directory
         * (see doc/browsing.txt for details).
         * NULL if the stream is not a directory.
        int         (*pf_readdir)(stream_t *, input_item_node_t *); 
    		/* ... */

Given the new, and awesome (thank you Remi), documentation in
`include/vlc_stream.h` it is clear that we have two mechanisms to
check whether a `stream_t*` refers to a directory or not - as such,
one of them is redundant.

I propose that we resolve the issue by either;

  - remove `STREAM_IS_DIRECTORY` and rely entirely on the state of
    `stream_t.pf_readdir`, or;

  - change the documented behavior (remove the constraint of
    `pf_readdir`), so that `STREAM_IS_DIRECTORY` is the mechanism
    to use.


The contents later in this post will contain my immediate reactions
relevant to this issue, but the purpose of this post is of course to
discuss the following:

  - What should we do about `STREAM_IS_DIRECTORY` and
    the constraints documented for `stream_t.pf_readdir`?


Note: `s->pf_readdir == NULL` is currently b0rked

The currently documented behavior does not correspond to what we
actually do when creating an access.

Below is the relevant snippet from `src/input/access.c`:

    stream_t *stream_AccessNew(vlc_object_t *parent, input_thread_t *input,
                               bool preparsing, const char *url)
    		/* ... */
        if (access->pf_readdir != NULL)
            s->pf_readdir = AStreamReadDir;
            s->pf_readdir = AStreamNoReadDir;
    		/* ... */


Immediate reactions

Given that we have two alternatives, this section will include my
thoughts on the different paths we can decide to walk.

I strongly feel that we should not have two separate mechanisms for
checking the same thing.

Implications of removing `STREAM_IS_DIRECTORY`

  - **Unable to privatize callbacks**
    It will be more complicated to (in the future) move `pf_readdir`
    (and the related callbacks) currently within `struct stream_t` to
    a private area not accessible by entities, outside the core/the
    module itself, using a stream.

  - **The codebase currently favors `STREAM_IS_DIRECTORY`**

    As previously stated in the *Note*, `stream_AccessNew` must comply
    with the mentioned documented behavior (it currently does not, and
    has not since (at least) d2e07d11 (2015-08-25).

  - **Conditional paths on `s->pf_readdir` are non-existant**
    As far as I can see, there is not a single place where the state
    of `s->pf_readdir` (where `s` can be any `stream_t*`) is used to
    check whether a stream is considered a directory or not.

Implications of removing the constraints of `pf_readdir`

  - **More difficult to check stream-"type"**

    It will require additional logic in order to check whether a
    stream is a directory or not. `if( s->pf_readdir )` is of course
    easier to write than the below:

        bool b_isdir;

        if( stream_Control( s, STREAM_IS_DIRECTORY, &b_isdir ) )
            b_isdir = false;

        if( b_isdir )
            /* ... */

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20160722/d8ffa134/attachment.html>

More information about the vlc-devel mailing list