[vlc-devel] [PATCH] upnp: fetch all media items correctly. (#6716)

[mirsal]

Hi Naohiro

Thanks for the patch !
A few comments inline.

On Mon, 2012-04-30 at 18:02 +0900, Naohiro KORIYAMA wrote:
> From 21a97945ce3e867a0856e6fc663b79f106f5b875 Mon Sep 17 00:00:00 2001
> From: Naohiro KORIYAMA <nkoriyama at gmail.com>
> Date: Mon, 30 Apr 2012 17:57:19 +0900
> Subject: [PATCH 17/17] upnp: fetch all media items correctly. (#6716)
> 
> ---
>  modules/services_discovery/upnp.cpp |   87
> +++++++++++++++++++++++++++++++----
>  modules/services_discovery/upnp.hpp |    2 +-
>  2 files changed, 78 insertions(+), 11 deletions(-)
> 
> diff --git a/modules/services_discovery/upnp.cpp
> b/modules/services_discovery/upnp.cpp
> index dc239f3..ff38462 100644
> --- a/modules/services_discovery/upnp.cpp
> +++ b/modules/services_discovery/upnp.cpp
> @@ -236,13 +242,47 @@ IXML_Document* parseBrowseResult( IXML_Document*
> p_doc )
>      IXML_Node* p_text_node = ixmlNode_getFirstChild( p_result_node );
>      if ( !p_text_node ) return 0;
>  
> -    const char* psz_result_string =
> ixmlNode_getNodeValue( p_text_node );
> +    return ixmlNode_getNodeValue( p_text_node );
> +}
> +
> +/*
> + * Extracts the result document from a SOAP response
> + */
> +IXML_Document* parseBrowseResult( IXML_Document* p_doc )
> +{
> +    ixmlRelaxParser( 1 );
> +
> +    const char* psz_result_string = xml_getChildElementValue( p_doc,
> "Result" );
> +    if( !psz_result_string ) return 0;
>  
>      IXML_Document* p_browse_doc = ixmlParseBuffer( psz_result_string
> );
>  
>      return p_browse_doc;
>  }
>  
> +/*
> + * Get the number value from a SOAP response
> + */
> +int parseBrowseNumberValue( IXML_Document* p_doc,
> +                            const char* psz_tag_name_ )
> +{
> +    ixmlRelaxParser( 1 );

You shouldn't need to relax the parser more than once,
does it fail if you remove that ?

> +    const char* psz_number_string = xml_getChildElementValue( p_doc,
> +
> psz_tag_name_ );
> +    if( !psz_number_string ) return 0;
> +
> +    char *psz_end;
> +    long l = strtol( psz_number_string, &psz_end, 10 );
> +    if( *psz_end || l < 0 || l > INT_MAX )
> +    {
> +        return 0;
> +    }
> +    else
> +    {
> +        return (int)l;
> +    }
> +}
>  
>  /*
>   * Handles all UPnP events
> @@ -709,7 +749,7 @@ void MediaServer::fetchContents()
>  
>      Container* root = new Container( 0, "0", getFriendlyName() );
>  
> -    _fetchContents( root );
> +    _fetchContents( root, 0 );
>  
>      _p_contents = root;
>      _p_contents->setInputItem( _p_input_item );
> @@ -720,7 +760,7 @@ void MediaServer::fetchContents()
>  /*
>   * Fetches and parses the UPNP response
>   */
> -bool MediaServer::_fetchContents( Container* p_parent )
> +bool MediaServer::_fetchContents( Container* p_parent, int
> i_starting_index )
>  {
>      if (!p_parent)
>      {
> @@ -728,9 +768,21 @@ bool MediaServer::_fetchContents( Container*
> p_parent )
>          return false;
>      }
>  
> +    char* psz_starting_index;
> +    if( asprintf( &psz_starting_index, "%d", i_starting_index ) < 0 )
> +    {
> +        msg_Err( _p_sd, "asprintf error:%d", i_starting_index );
> +        return false;
> +    }
> +
>      IXML_Document* p_response =
> _browseAction( p_parent->getObjectID(),
>                                        "BrowseDirectChildren",
> -                                      "*", "0", "0", "" );
> +                                      "*", /* Filter */
> +                                      psz_starting_index, /*
> StartingIndex */
> +                                      "0", /* RequestedCount */
> +                                      "" /* SortCriteria */
> +                                      );
> +    free( psz_starting_index );
>      if ( !p_response )
>      {
>          msg_Err( _p_sd, "No response from browse() action" );
> @@ -738,6 +790,12 @@ bool MediaServer::_fetchContents( Container*
> p_parent )
>      }
>  
>      IXML_Document* p_result = parseBrowseResult( p_response );
> +    int i_number_returned = parseBrowseNumberValue( p_response,
> "NumberReturned" );
> +    int i_total_matches = parseBrowseNumberValue( p_response ,
> "TotalMatches" );

Please use #define for tag names.

> +#ifndef NDEBUG
> +    msg_Dbg( _p_sd, "i_starting_index[%d]i_number_returned[%
> d]_total_matches[%d]\n",
> +             i_starting_index, i_number_returned, i_total_matches );
> +#endif
>      ixmlDocument_free( p_response );
>  
>      if ( !p_result )
> @@ -777,7 +835,7 @@ bool MediaServer::_fetchContents( Container*
> p_parent )
>  
>              Container* container = new Container( p_parent, objectID,
> title );
>              p_parent->addContainer( container );
> -            _fetchContents( container );
> +            _fetchContents( container, 0 );
>          }
>          ixmlNodeList_free( containerNodeList );
>      }
> @@ -833,6 +891,15 @@ bool MediaServer::_fetchContents( Container*
> p_parent )
>      }
>  
>      ixmlDocument_free( p_result );
> +
> +    if( i_starting_index + i_number_returned < i_total_matches )
> +    {
> +        if( !_fetchContents( p_parent, i_starting_index +
> i_number_returned ) )
> +        {
> +            return false;
> +        }
> +    }
> +
>      return true;
>  }

I'd rather have:

if( i_starting_index + i_number_returned < i_total_matches )
{
    return _fetchContents( p_parent,
                           i_starting_index + i_number_returned );
}

return true;

Best regards,
> 
-- 
mirsal 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 490 bytes
Desc: This is a digitally signed message part
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20120430/834d8c20/attachment.sig>