[vlc-devel] [PATCH 01/12] input item: expose locked version of GetMeta()

[Romain Vimont]

On Fri, Oct 12, 2018 at 11:46:35PM +0200, Romain Vimont wrote:
> On Fri, Oct 12, 2018 at 10:44:18PM +0300, Rémi Denis-Courmont wrote:
> > Le perjantaina 12. lokakuuta 2018, 0.14.39 EEST Romain Vimont a écrit :
> > > In order to sort the playlist correctly, the input items must be locked
> > > across several calls to GetMeta() (among others).
> > 
> > Taking multiple locks from the same class simultaneously is a terrible idea 
> > that will defeat lock dependency checkers (no that we use any, but we probably 
> > should).
> 
> I agree. I wrote it that way (patch 7) because I wanted all these
> conditions simultaneously:
>  - _Sort() returning void (which implies sorting in place without memory
>    allocation);
>  - not unnecessarily inefficient (no lock/unlock on every compare());
>  - a correct sorting (the items must not be updated during sorting).
> 
> > And it is theoretically only possible if there is an order, which this patch 
> > suspiciously fails to define.
> 
> The input items instances are not supposed to be used in several
> playlists or services discoveries simultaneously (we copy them in that
> case), and in practice, _Sort() should be the only function which may
> lock several items, so lock-order inversion should never happen.
> 
> But yes, in theory, the clients may do what they want with input items,
> including locking several of them, making lock-order inversion possible.
> So it's quite fragile.
> 
> The alternative I have in mind is:
>  1. make _Sort() return a status (int) to report allocation failure;
>  2. create a separate temporary vector of a custom struct, containing
>     the useful properties of input_item_t and a reference to the
>     playlist item;
>  3. for each item, copy (with lock held) the properties which are
>     relevant for the requested sorting;
>  4. sort that temporary vector;
>  5. report the resulting order to the playlist items vector.
> 
> What do you think?

Yet another alternative would be to copy the input item properties on
playlist item creation and update them on preparsing event.