[vlc-devel] [PATCH] Fixed libvlc_vlm_show_media to return a string representation of a vlm media again

[Laurent Aimar]

Hi,

On Wed, Mar 11, 2009, Alexander Bethke wrote:
> +static vlm_media_instance_t *libvlc_vlm_get_media_instance( libvlc_instance_t *p_instance,
> +                                                            const char *psz_name,
> +                                                            int i_minstance_idx,
> +                                                            libvlc_exception_t *p_exception )
> +{
> +    vlm_t *p_vlm;
> +    vlm_media_instance_t **pp_minstance;
> +    vlm_media_instance_t *p_minstance;
> +    int i_minstance;
> +    int64_t id;
> +
> +    VLM_RET(p_vlm, NULL);
> +
> +    if( vlm_Control( p_vlm, VLM_GET_MEDIA_ID, psz_name, &id ) ||
> +        vlm_Control( p_vlm, VLM_GET_MEDIA_INSTANCES, id, &pp_minstance, &i_minstance ) )
> +    {
> +        libvlc_exception_raise( p_exception, "Unable to get %s instances", psz_name );
> +        return NULL;
> +    }
> +    p_minstance = NULL;
> +    if( i_minstance_idx >= 0 && i_minstance_idx < i_minstance )
> +    {
> +        p_minstance = pp_minstance[i_minstance_idx];
> +        TAB_REMOVE( i_minstance, pp_minstance, p_minstance );
> +    }
> +    while( i_minstance > 0 )
> +        vlm_media_instance_Delete( pp_minstance[--i_minstance] );
It would be cleaner to use an extra variable like
for( int i = 0; i < i_minstance; i++ )
    vlm_media_instance_Delete( pp_minstance[i] );
as i_minstance is the number of element.
(Cosmetic)

>  /* local function to be used in libvlc_vlm_show_media only */
>  static char* recurse_answer( char* psz_prefix, vlm_message_t *p_answer ) {
>      char* psz_childprefix;
> -    char* psz_response="";
> +    char* psz_response;
>      char* response_tmp;
>      int i;
>      vlm_message_t *aw_child, **paw_child;
>  
> +    asprintf( &psz_response, "");
You need to check asprintf return value.
>      asprintf( &psz_childprefix, "%s%s.", psz_prefix, p_answer->psz_name );
 And while you are at it, if you could add it for existing asprintf it
would be cool :)

>  
>      if ( p_answer->i_child )
> @@ -65,96 +118,40 @@ static char* recurse_answer( char* psz_prefix, vlm_message_t *p_answer ) {
>      return psz_response;
>  }
>  
> -char* libvlc_vlm_show_media( libvlc_instance_t *p_instance, char *psz_name,
> +/*  Returning a string representation of the media I find should be changed to
> +    either refactoring this to return some sort of struct or or to add more
> +    libvlc_vlm_get_media_instance_x methods to make available all information
> +    that you get out of the string representation.
> +    -Alexander Bethke */
> +char* libvlc_vlm_show_media( libvlc_instance_t *p_instance,
> +                             const char *psz_name,
>                               libvlc_exception_t *p_exception )
>  {
>      char *psz_message;
>      vlm_message_t *answer;
>      char *psz_response;
> +    vlm_t *p_vlm;
> +
> +    VLM_RET(p_vlm, NULL);
>  
> -    CHECK_VLM;
>      asprintf( &psz_message, "show %s", psz_name );
>      asprintf( &psz_response, "", psz_name );
 Same.
> -    vlm_ExecuteCommand( p_instance->p_vlm, psz_message, &answer );
> +    vlm_ExecuteCommand( p_vlm, psz_message, &answer );
vlm_ExecuteCommand has a return value that need checking.

>      if( answer->psz_value )
>      {
>          libvlc_exception_raise( p_exception, "Unable to call show %s: %s",
>                                  psz_name, answer->psz_value );
>      }
> -    else
> -    {
> +    else {
 It would be better if you keep the { placement coherent.

>          if ( answer->child )
>          {
>              psz_response = recurse_answer( "", answer );
>          }
>      }
> +
>      free( psz_message );
>      return(psz_response );
>  }

Regards,

-- 
fenrir