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

[Naohiro KORIYAMA]

Thank you for reviewing.

2012/4/30 mirsal <mirsal at mirsal.fr>:
> 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 ?

I didn't try it.

>
>> +    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.

Indeed.

>
>> +#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;

Yes. I think so too.

I'll modify next time. (the patch has already been applied.)

Best regards,
-- 
KORIYAMA, Naohiro
nkoriyama at gmail.com