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

[Romain Vimont]

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?