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

[Jakub Wieczorek]

Hi,

Thanks for the review.

2011/3/13 Ilkka Ollakka <ileoo at videolan.org>:
> On Sun, Mar 13, 2011 at 11:24:11AM +0100, Jakub Wieczorek wrote:
>> Hello,
>
>> I'm attaching the latest version of the last.fm access module branch.
>> It's been updated per the recent XML reader changes.
>> The branch had not been merged in before due to uncertainty over
>> conformity of this module to the ToS of last.fm's API. Jean told me
>> that it should be safe to get this now.
>
>> I'd love someone to take a look once again. Thanks.
>
> Hi,
>
>> Subject: [PATCH 1/2] Add an access module that works with last.fm radio stations.
>
>> --- /dev/null
>> +++ b/modules/access/lastfm.c
>
>> +/*****************************************************************************
>> + * Authenticates: performs the authentication of the user
>> + *****************************************************************************/
>> +// FIXME: We should be doing this only once by storing the session key
>> +// persistently and then reusing next time it's needed.
>> +static int Authenticate( access_t *p_access )
>> +{
>> +    char *psz_lastfm_username = var_InheritString( p_access, "lastfm-username" );
>> +    char *psz_lastfm_password = var_InheritString( p_access, "lastfm-password" );
>> +    char *psz_auth_token = NULL;
> ...
>> +    if( !psz_lastfm_username || !psz_lastfm_password ||
>> +        *psz_lastfm_username == '\0' || *psz_lastfm_password == '\0' )
>> +    {
>> +        msg_Err( p_access, "last.fm credentials missing" );
>> +        i_retcode = VLC_ENOVAR;
>> +        goto error;
>> +    }
>> +
>> +    psz_auth_token = AuthToken( psz_lastfm_username, psz_lastfm_password );
>> +    if( !psz_auth_token )
>> +    {
>> +        msg_Err( p_access, "out of memory" );
>> +        i_retcode = VLC_ENOMEM;
>> +        goto error;
>> +    }
>> +
>> +    msg_Dbg( p_access, "username: %s; password: %s; auth token: %s",
>> +             psz_lastfm_username, psz_lastfm_password, psz_auth_token );
>
> I think it's not that good idea to log password in plaintext, and you
> could free psz_lastfm_password in this point too?

Definitely.

> Also why don't add
> some logic and just store generated md5 hash to lastfm-password instead
> keep it plaintext (Yes I know we don't do that in last.fm submit plugin
> either, just an idea).

Well, I must agree that storing user credentials in plain text is
probably a bad idea. But I fear the change you recommend isn't
possible with the current variable mechanism - simply put, the user
fills in the login data in plain text and they get directly saved in
such a form. I can't think of any way to do the UI part of this right
with the current system either, once we decide to store the hash of
the password instead of the password itself.

Ideally, we could use the OS keyring to make sure the credentials are
safe and at the same time, not to reinvent the wheel to protect it.

>> +/*****************************************************************************
>> +* CurlWriteData: a callback that receives the data
>> +*****************************************************************************/
>> +size_t CurlWriteData( void *p_data, size_t i_size, size_t i_nmemb, void *p_userdata )
>> +{
>> +    i_buffersize = NextPowerOfTwo( p_buffer->i_used + i_realsize );
>
> Why not just something like ? Assuming that i_buffersize starts from
> some power of 2
> while i_buffersize <= (p_buffer->i_used+i_realsize )
>    i_buffersize <<= 1;

Yeah, that seems slightly easier to read.

>> +static stream_t *Request( access_t *p_access, struct url_argument_t *p_arguments, int i_method )
>> +{
>> +    msg_Dbg( p_access, "Issuing a %s request...", i_method == REST_GET ? "GET" : "POST" );
>> +
>> +
>> +    p_curl_handle = curl_easy_init();
>> +    curl_easy_setopt( p_curl_handle, CURLOPT_HTTP_VERSION, CURL_HTTP_VERSION_1_1 );
>> +    curl_easy_setopt( p_curl_handle, CURLOPT_URL, psz_url );
>> +    curl_easy_setopt( p_curl_handle, CURLOPT_USERAGENT, "VLC media player" );
>> +    curl_easy_setopt( p_curl_handle, CURLOPT_WRITEFUNCTION, CurlWriteData );
>> +    curl_easy_setopt( p_curl_handle, CURLOPT_WRITEDATA, p_access );
>> +    if( psz_data )
>> +        curl_easy_setopt( p_curl_handle, CURLOPT_POSTFIELDS, psz_data );
>
> Is it really harder/requires more code to do http post by hand over
> TCP-connection?

It's not that much harder.
It does require more code.
It reinvents the wheel. Again.

Yes, I'm not a fan of adding a new dependency either. But at the same
time, I'm also more opposed to writing the same piece of code again,
where we could just use something that works and is perfectly
optimized for the job it does.

I'll try to see how much the vlc binary would grow with the curl
library (with the unuseful stuff compiled out).
Is the binary size the only concern? Would adding a curl dependency be
a maintainance overhead?

The truth is I'm new to the project and if this particular choice will
affect it in the long-term, it's clear that the call belongs to the
more involved community members such as yourself.

>> +        p_argument->psz_name = curl_escape( p_argument->psz_name, strlen( p_argument->psz_name ) );
>> +        p_argument->psz_value = curl_escape( p_argument->psz_value, strlen( p_argument->psz_value ) );
>
> encode_URI_component?
> and maybe vlc_url_t is the struct youre looking for?

Hm, I'm trying to recall why I ended up using curl_escape instead of
encode_URI_component (this code is 2 months old and my memory is
vague).
Re: vlc_uri_t. This structure is useful for parsing URIs into a nice
structure. In my case I want to do the opposite, which vlc_uri_t can't
help me with.

Best regards,
Jakub Wieczorek