[vlc-devel] [GCI] last.fm access module

[Jakub Wieczorek]

Hello,

Thanks for the review.

2011/1/2 Tobias Güntner <fatbull at web.de>:
> Am 02.01.2011 00:04, schrieb Jakub Wieczorek:
>> +static int Open( vlc_object_t *p_this )
> ...
>> +    curl_global_init( CURL_GLOBAL_ALL );
> ...
>> +static void Close( vlc_object_t *p_this )
> ...
>> +    curl_global_cleanup();
>
> I think it is a bad idea to call global init/cleanup here. This might
> fail mysteriously if another module ever uses libcurl.

Invoking these functions multiple times is safe, further calls simply
don't do anything.
But I may need to protect the calls, as activation callbacks for
different modules are not necessarily run from the same thread.

> Furthermore, the
> man page says those functions are unsafe if more than one thread is
> running (even if only one thread actually uses libcurl).

Yes. The man page sounds a bit grim but according to my quick
investigation, they are unsafe due to some underlying libraries being
unsafe. These are:
GnuTLS - this one is already protected with a locking callback
provided to the library.
OpenSSL - this one isn't used in VLC and if it's ever used I'd let the
one who introduces it take care of the multi-threading issues as it's
not stricly specific to libcurl.
NSS - ditto
PolarSSL - ditto
yassl - ditto

>
>> +static int Control( access_t *p_access, int i_query, va_list args )
> ...
>> +            pb_bool = ( bool * )va_arg( args, bool* );
> ...
>> +            pb_bool = ( bool * )va_arg( args, bool* );
> ...
>> +            pi_64 = ( int64_t * )va_arg( args, int64_t * );
>
> Type casts are not necessary here.

Fixed.

>
>> +static inline uint64_t NextPowerOfTwo( uint64_t i_value )
>> +{
>
> --i_value;
>
>> +    i_value = (i_value >> 1) | i_value;
>> +    i_value = (i_value >> 2) | i_value;
>> +    i_value = (i_value >> 4) | i_value;
>> +    i_value = (i_value >> 8) | i_value;
>> +    i_value = (i_value >> 16) | i_value;
>> +    i_value = (i_value >> 32) | i_value;
>
> ++i_value;
>
>> +    return i_value;
>> +}

Oops. :) Fixed.

Best regards,
Jakub Wieczorek
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Add-an-access-module-that-works-with-last.fm-radio-s.patch
Type: text/x-patch
Size: 42403 bytes
Desc: not available
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20110102/43820422/attachment.bin>