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

[Filip Roséen]

Introduction
============

    /**
     * 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.


Discussion
----------

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;
        else
            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>