[vlc-devel] [PATCH 2/3] Audioscrobbler: Update the scrobbling protocol to the version 2.0.

[Tobias Güntner]

Hello!

Am 11.03.2012 12:05, schrieb samuel.pitoiset at gmail.com:
> +        AddParam(&p_params, strdup(psz_key), strdup(psz_val));
...

What if strdup returns NULL?

> +    /* build the HTTP request */
> +    psz_request = BuildSignedRequest(p_this,&p_params, POST);
> +    vlc_array_clear(&p_params);

Won't this leak the strdup()ed key/value pairs from above and malloc()ed 
audioscrobbler_param_t structs from AddParam()?

> +    if (strcasecmp(node, "lfm"))
...
 > +        if (strcasecmp(attr, "status"))

Is strcasecmp really required? XML node names and attribute names are 
usually case sensitive.

> +            /* an error occured */
> +            if (!strcasecmp(value, "4"))

Case insensitive numbers? ;)

> +            {
> +                /* authentication failed, bad username/password combination */
> +                msg_Err(p_this, "last.fm username or password is incorrect.");
> +            }
> +
> +            if (!strcasecmp(value, "10"))
> +            {
> +                /* invalid API key */
> +                msg_Err(p_this, "You must be granted a valid key by last.fm");
> +            }
> +
> +            if (!strcasecmp(value, "26"))
> +            {
> +                /* suspend API key */
> +                msg_Err(p_this, "Access for your account has been suspended.");
> +            }

Might be a good idea to convert this to an if/else if chain and add a 
generic log message in the final else branch.

> +
> +            goto proto;
> +        }
> +    }

The "send request and check response status" parts in the first patch 
and in this patch have a lot in common. Perhaps this could be refactored?

Regards,
Tobias