[vlc-devel] [PATCH] GSoC: MTP Devices initial release

[Fabio Ritrovato]

> I think that changing the psz_path variable here is very ugly.
> Is it needed ? If not store it in access_sys_t.
> Then about tempnam, it is a bad choice because it does not prevent a race condition
> and you have to delete it afterward.
> I am not sure which one will be the most appropriate for your case.
> (tmpfile, utf8_mkstemp, .. ) It depends on your library needed or not the path.

I tried storing it in p_sys, but then Taglib complains it cannot open
the file (since p_access->psz_path is not a file).
Also, http.c changes psz_path too, so I think it could be ok like this...

About the file, I need the path to be passed to libmtp, so the ones
you suggested are not good...
Are there any others?

> You coul replace the break by return VLC_SUCCESS;

What's the point? break is shorter than VLC_SUCCESS :P

>>> +    /* Update default_pts to a suitable value for file access */
>>> +    var_Create( p_access, "file-caching",
>> VLC_VAR_INTEGER|VLC_VAR_DOINHERIT );
>>  I think "mtp-caching" was meant here.
>
> Maybe not. It's reading a regular file, so why create yet another caching
> parameter?

I think he refers to the fact i used add_integer( "mtp-caching", ... )
in the module description...
I removed it and left file-caching in the code, is that ok?

>>> +        /* Delay a bit to avoid consuming all the CPU. This is
>> particularly
>>> +         * useful when reading from an unconnected FIFO. */
>>> +        msleep( INPUT_ERROR_SLEEP );
>>  I think a poll() could be used here or maybe net_Read (like file.c) as
>> you used O_NONBLOCK
>
> There is no such thing as non-blocking I/O for _regular_ files.
> Specifically, "Regular files shall always poll TRUE for reading and
> writing."
> Setting the O_NONBLOCK flag has _absolutely_ _no_ effects on a regular file
> descriptor.
>
> Non-blocking I/O works for pipes and FIFOs, sockets, terminals and
> pseudo-terminals, STREAMS, and on platform-specific basis, select device
> files.
>
> The msleep() should simply be removed, and the error should be considered
> fatal and passed onto the stream layer. IIRC guess that means setting b_eof
> to true and returning 0.

The error is already considered fatal inside the switch, in the
default case, and treated as you suggested.
msleep() is used when errno is EINTR or EAGAIN, but i don't think they
should be considered fatal errors.