[vlc-devel] [PATCH] Refactored libvlc_vlm_show_media

[Rémi Denis-Courmont]

Le lundi 18 mai 2009 17:46:13 Alexander Bethke, vous avez écrit :
> +/* local function to be used in libvlc_vlm_show_media only */
> +static char* recurse_answer( vlm_message_t *p_answer, const char*
> psz_delim,
> +                             const int i_list ) {
> +    char* psz_childdelim = NULL;
> +    char* psz_response = NULL;
> +    char* psz_nametag = NULL;
> +    int i_success = 0;
> +    int i;
> +    vlm_message_t *aw_child, **paw_child;
> +
> +    psz_response = strdup( "" );
> +    i_success = asprintf( &psz_childdelim, "%s\t", psz_delim);
> +
> +    /* starting with the children of root node */
> +    if( i_success != -1 && p_answer->i_child )
> +    {
> +        paw_child = p_answer->child;
> +        aw_child = *( paw_child );
> +        /* Iterate over children */
> +        for( i = 0; i < p_answer->i_child; i++ )
> +        {
> +            /* Spare comma if it is the last element */
> +            char c_comma = ',';
> +            if( i == (p_answer->i_child - 1) )
> +                c_comma = ' ';
> +
> +            /* Append name of child node, if not in a list */
> +            if( !i_list )
> +            {
> +                i_success = asprintf( &psz_response, "%s\"%s\": ",
> +                              psz_response, aw_child->psz_name );
> +                if( i_success == -1 ) break;
> +            }
> +
> +            /* If child node has children, */
> +            if( aw_child->i_child )
> +            {
> +                /* If the parent node is a list (hence the child node is
> +                 * inside a list), create a property of its name as if it
> +                 * had a name value node
> +                 */
> +                if( i_list )
> +                {
> +                    i_success = asprintf( &psz_nametag, "\"name\":
> \"%s\",%s", +                                  aw_child->psz_name,
> psz_childdelim ); +                    if( i_success == -1 ) break;
> +                }
> +                else
> +                {
> +                    psz_nametag = strdup( "" );
> +                }
> +                /* If the child is a list itself, format it accordingly
> and +                 * recurse through the child's children, telling them
> that +                 * they are inside a list.
> +                 */
> +                if( strcmp( aw_child->psz_name, "media" ) == 0 ||
> +                    strcmp( aw_child->psz_name, "inputs" ) == 0 ||
> +                    strcmp( aw_child->psz_name, "options" ) == 0 )
> +                {
> +                    i_success = asprintf( &psz_response, "%s[%s%s%s]%c%s",
> +                                          psz_response, psz_childdelim,
> +                                          recurse_answer( aw_child,
> +                                                          psz_childdelim,
> 1 ), +                                          psz_delim, c_comma,
> psz_delim ); +                    if( i_success == -1 ) break;
> +                }
> +                /* Not a list, so format the child as a JSON object and
> +                 * recurse through the child's children
> +                 */
> +                else
> +                {
> +                    i_success = asprintf( &psz_response,
> "%s{%s%s%s%s}%c%s", +                                         
> psz_response, psz_childdelim, psz_nametag, +                               
>           recurse_answer( aw_child, +                                      
>                    psz_childdelim, 0 ), +                                  
>        psz_delim, c_comma, psz_delim ); +                    if( i_success
> == -1 ) break;
> +                }
> +            }
> +            /* Otherwise - when no children are present - the node is a
> +             * value node. So print the value string
> +             */
> +            else
> +            {
> +                /* If value is equivalent to NULL, print it as null */
> +                if( aw_child->psz_value == NULL
> +                    || strcmp( aw_child->psz_value, "(null)" ) == 0 )
> +                {
> +                    i_success = asprintf( &psz_response, "%snull%c%s",
> +                                          psz_response, c_comma, psz_delim
> ); +                    if( i_success == -1 )
> +                        break;
> +                }
> +                /* Otherwise print the value in quotation marks */
> +                else
> +                {
> +                    i_success = asprintf( &psz_response, "%s\"%s\"%c%s",
> +                                          psz_response,
> aw_child->psz_value, +                                          c_comma,
> psz_delim );
> +                    if( i_success == -1 ) break;
> +                }
> +            }
> +            /* getting next child */
> +            paw_child++;
> +            aw_child = *( paw_child );
> +        }
> +    }
> +    free( psz_nametag );
> +    free( psz_childdelim );

When asprintf() returns -1, the referenced string pointer is undefined, so you 
can't blindly free() it.

> +    if( i_success == -1 ) {
> +        free( psz_response );
> +        psz_response = strdup( "" );
> +    }
> +    return psz_response;
> +}
> +
> +const char* libvlc_vlm_show_media( libvlc_instance_t *p_instance,
> +                                   const char *psz_name,
> +                                   libvlc_exception_t *p_exception )
> +{
> +    char *psz_message = NULL;
> +    vlm_message_t *answer = NULL;
> +    char *psz_response = NULL;
> +    const char *psz_fmt = NULL;
> +    const char *psz_delimiter = NULL;
> +    int i_list;
> +    vlm_t *p_vlm = NULL;
> +
> +    VLM_RET(p_vlm, NULL);
> +
> +    if( psz_name == NULL )
> +    {
> +        libvlc_exception_raise( p_exception, "No media name supplied" );
> +    }
> +    else if( asprintf( &psz_message, "show %s", psz_name ) == -1 )
> +    {
> +        libvlc_exception_raise( p_exception, "Unable to call show %s",
> +                                psz_name );
> +    }
> +    else
> +    {
> +        vlm_ExecuteCommand( p_vlm, psz_message, &answer );
> +        if( answer->psz_value )
> +        {
> +            libvlc_exception_raise( p_exception, "Unable to call show %s:
> %s", +                                    psz_name, answer->psz_value ); + 
>       }
> +        else if ( answer->child ) {
> +            /* in case everything was requested  */
> +            if ( strcmp( psz_name, "" ) == 0 )
> +            {
> +                psz_fmt = "{\n\t%s\n}\n";
> +                psz_delimiter = strdup( "\n\t" );
> +                i_list = 0;
> +            }
> +            else
> +            {
> +                psz_fmt = "%s\n";
> +                psz_delimiter = strdup( "\n" );
> +                i_list = 1;
> +            }
> +            if( asprintf( &psz_response, psz_fmt,
> +                          recurse_answer( answer, psz_delimiter, i_list )
> ) +                == -1 )
> +            {
> +                libvlc_exception_raise( p_exception, "Error in show %s",
> +                                        psz_name );
> +            }

I guess you're leaking psz_delimiter here. You should simply not call 
strdup().

> +        }
> +    }
> +    free( psz_message );
> +    return( psz_response );
> +}
>
>  void libvlc_vlm_release( libvlc_instance_t *p_instance,
>                           libvlc_exception_t *p_exception)

You are not taking any lock anywhere. Did you check that you were accessing 
only thread-specific or read-only data? (I don't know VLM very well)

-- 
Rémi Denis-Courmont
http://www.remlab.net/