<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Transitional//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd">
<html xmlns="http://www.w3.org/1999/xhtml">
<head>
  <meta http-equiv="Content-Type" content="text/html; charset=utf-8" />
  <meta http-equiv="Content-Style-Type" content="text/css" />
  <meta name="generator" content="pandoc" />
  <title></title>
  <style type="text/css">code{white-space: pre;}</style>
</head>
<body>
<p>Hi Hugo,</p>
<p>Thank you for your detailed review, all of the discovered issues will be addressed in a revised patch as soon as possible!</p>
<p>On 2017-01-16 14:43, Hugo Beauzée-Luyssen wrote:</p>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> On 01/12/2017 07:34 PM, Filip Roséen wrote:</code></pre>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> ---
  NEWS                                 |   1 +
  modules/stream_extractor/Makefile.am |   8 +
  modules/stream_extractor/archive.c   | 686 +++++++++++++++++++++++++++++++++++
  po/POTFILES.in                       |   1 +
  4 files changed, 696 insertions(+)
  create mode 100644 modules/stream_extractor/archive.c

 diff --git a/NEWS b/NEWS
 index 8aba9cfc4d..6565dd889e 100644
 --- a/NEWS
 +++ b/NEWS
 @@ -118,6 +118,7 @@ Stream filter:
   * Added ADF stream filter
   * Added ARIB STD-B25 TS streams decoder
   * Added stream prebuffering plugin
 + * Rewrite libarchive module as a stream_extractor
   * Removed HTTP Live streaming stream filter
   * Added zlib (a.k.a. deflate) decompression filter

 diff --git a/modules/stream_extractor/Makefile.am b/modules/stream_extractor/Makefile.am
 index e69de29bb2..07871fd4b6 100644
 --- a/modules/stream_extractor/Makefile.am
 +++ b/modules/stream_extractor/Makefile.am
 @@ -0,0 +1,8 @@
 +stream_extractordir = $(pluginsdir)/stream_extractor
 +stream_extractor_LTLIBRARIES =
 +
 +libarchivefuck_plugin_la_SOURCES = stream_extractor/archive.c
 +libarchivefuck_plugin_la_CFLAGS = $(AM_CFLAGS) $(ARCHIVE_CFLAGS)
 +libarchivefuck_plugin_la_LIBADD = $(ARCHIVE_LIBS)
 +stream_extractor_LTLIBRARIES += libarchivefuck_plugin.la
 +
 diff --git a/modules/stream_extractor/archive.c b/modules/stream_extractor/archive.c
 new file mode 100644
 index 0000000000..78781b0217
 --- /dev/null
 +++ b/modules/stream_extractor/archive.c
 @@ -0,0 +1,686 @@
 +
 +/*****************************************************************************
 + * archive.c: libarchive based stream filter
 + *****************************************************************************
 + * Copyright (C) 2016 VLC authors and VideoLAN
 + * $Id$</code></pre>
</blockquote>
<pre><code> Probably not required anymore. Also, happy new year :)</code></pre>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> + *
 + * Authors: Filip Roséen <filip@atch.se>
 + *
 + * This program is free software; you can redistribute it and/or modify it
 + * under the terms of the GNU Lesser General Public License as published by
 + * the Free Software Foundation; either version 2.1 of the License, or
 + * (at your option) any later version.
 + *
 + * This program is distributed in the hope that it will be useful,
 + * but WITHOUT ANY WARRANTY; without even the implied warranty of
 + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
 + * GNU Lesser General Public License for more details.
 + *
 + * You should have received a copy of the GNU Lesser General Public License
 + * along with this program; if not, write to the Free Software Foundation,
 + * Inc., 51 Franklin Street, Fifth Floor, Boston MA 02110-1301, USA.
 + *****************************************************************************/
 +
 +#ifdef HAVE_CONFIG_H</code></pre>
</blockquote>
<pre><code> nitpick, but I think the correct check is "#if"</code></pre>
</blockquote>
<pre><code>+/vlc/git> git grep "#if\s\+HAVE_CONFIG_H" | wc -l
1
+/vlc/git% git grep "#ifdef\s\+HAVE_CONFIG_H" | wc -l 
1131</code></pre>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> +#  include "config.h"
 +#endif
 +
 +#include <vlc_common.h>
 +#include <vlc_plugin.h>
 +#include <vlc_stream.h>
 +#include <vlc_stream_extractor.h>
 +#include <vlc_dialog.h>
 +#include <vlc_input_item.h>
 +
 +#include <assert.h>
 +#include <archive.h>
 +#include <archive_entry.h>
 +
 +static int     Open( vlc_object_t* );
 +static ssize_t Read( stream_extractor_t*, void*, size_t );
 +static int  ReadDir( stream_extractor_t*, input_item_node_t* );
 +static int  Control( stream_extractor_t*, int, va_list );
 +static int     Seek( stream_extractor_t*, uint64_t );
 +static void   Close( vlc_object_t* );
 +
 +vlc_module_begin()
 +    set_category( CAT_INPUT )
 +    set_subcategory( SUBCAT_INPUT_STREAM_FILTER )
 +    set_capability( "stream_extractor", 99 )
 +    set_description( N_( "libarchive based stream extractor" ) )
 +    set_callbacks( Open, Close )
 +vlc_module_end()
 +
 +/* ------------------------------------------------------------------------- */
 +
 +typedef struct archive libarchive_t;
 +
 +static         int libarchive_exit_cb( libarchive_t*, void* );
 +static         int libarchive_jump_cb( libarchive_t*, void*, void* );
 +static  la_int64_t libarchive_skip_cb( libarchive_t*, void*, off_t );
 +static  la_int64_t libarchive_seek_cb( libarchive_t*, void*, la_int64_t, int );
 +static  la_ssize_t libarchive_read_cb( libarchive_t*, void*, const void** );
 +
 +/* ------------------------------------------------------------------------- */
 +
 +typedef struct private_sys_t private_sys_t;
 +
 +static int probe( stream_extractor_t* );
 +static int setup( stream_extractor_t*, char* );
 +
 +static int archive_init( stream_extractor_t* );
 +static int archive_clean( stream_extractor_t* );
 +
 +static int archive_seek_subentry( stream_extractor_t*, char const* psz_subentry );
 +static int archive_push_resource( private_sys_t*, stream_t*, char* );</code></pre>
</blockquote>
<pre><code> Things probably could be reordered to avoid the forward declarations</code></pre>
</blockquote>
<p>I actually prefer having forward declarations instead reordering, but given that there seem to be more developers that prefer the latter in the codebase, I will address this issue while I fix the other stuff.</p>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> +
 +struct libarchive_callback_t {
 +    private_sys_t* p_sys;
 +    stream_t* p_source;
 +    char* psz_url;
 +};
 +
 +typedef struct libarchive_callback_t libarchive_callback_t;
 +
 +struct private_sys_t
 +{
 +    libarchive_t* p_archive;
 +    stream_extractor_t* p_extractor;
 +
 +    struct archive_entry* p_entry;
 +
 +    uint64_t i_offset;
 +
 +    uint8_t buffer[ 8096 ];</code></pre>
</blockquote>
<pre><code> Any reason for this value?</code></pre>
</blockquote>
<p>Other than being <code>2^N</code> and a reasonable size in terms of buffering, not really. Do you have some other buffer size in mind?</p>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> +    bool b_seekable_source;
 +    bool b_seekable_archive;
 +
 +    libarchive_callback_t** pp_callback_data;
 +    size_t i_callback_data;
 +
 +    char* psz_additional_files;
 +};
 +
 +/* ------------------------------------------------------------------------- */
 +
 +static int Open( vlc_object_t* p_obj )
 +{
 +    stream_extractor_t* p_extractor = (void*)p_obj;
 +
 +    if( probe( p_extractor ) )
 +        return VLC_EGENERIC;
 +
 +    if( setup( p_extractor, var_InheritString( p_extractor, "concat-list" ) ) )</code></pre>
</blockquote>
<pre><code> This is leaking the returned string (see remarks about setup function)</code></pre>
</blockquote>
<p>Well spotted, thank you.</p>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> +        return VLC_EGENERIC;
 +
 +    if( archive_init( p_extractor ) )
 +        goto error;
 +
 +    if( p_extractor->identifier )
 +    {
 +        if( archive_seek_subentry( p_extractor, p_extractor->identifier ) )
 +          goto error;
 +
 +        p_extractor->stream.pf_read = Read;
 +        p_extractor->stream.pf_control = Control;
 +        p_extractor->stream.pf_seek = Seek;
 +    }
 +    else
 +        p_extractor->directory.pf_readdir = ReadDir;
 +
 +    return VLC_SUCCESS;
 +
 +error:
 +    Close( p_obj );
 +    return VLC_EGENERIC;
 +}
 +
 +static int Control( stream_extractor_t* p_extractor, int i_query, va_list args )
 +{
 +    private_sys_t* p_sys = p_extractor->p_sys;
 +
 +    switch( i_query )
 +    {
 +        case STREAM_IS_DIRECTORY:
 +            *va_arg( args, bool* ) = false;</code></pre>
</blockquote>
<pre><code> I don't know this API much, but does it make sense to have STREAM_IS_DIRECTORY returning
 false at all time but still provide a pf_readdir ?</code></pre>
</blockquote>
<p>The <code>stream->pf_readdir</code> vs <code>STREAM_IS_DIRECTORY</code> is b0rked, and I started a discussion about it a few months back but that thread did not receive any attention.</p>
<ul>
<li>https://mailman.videolan.org/pipermail/vlc-devel/2016-July/108687.html</li>
</ul>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> +            return VLC_EGENERIC;
 +
 +        case STREAM_CAN_FASTSEEK:
 +            *va_arg( args, bool* ) = false;
 +            break;
 +
 +        case STREAM_CAN_SEEK:
 +            *va_arg( args, bool* ) = p_sys->p_entry && p_sys->b_seekable_source;
 +            break;
 +
 +        case STREAM_GET_SIZE:
 +            *va_arg( args, uint64_t* ) = archive_entry_size( p_sys->p_entry );
 +            break;
 +
 +        default:
 +            return vlc_stream_vaControl( p_extractor->source, i_query, args );
 +    }
 +
 +    return VLC_SUCCESS;
 +}
 +
 +static int ReadDir( stream_extractor_t* p_extractor, input_item_node_t* p_node )
 +{
 +    private_sys_t* p_sys = p_extractor->p_sys;
 +    libarchive_t* p_arc = p_sys->p_archive;
 +
 +    struct archive_entry* entry;
 +    int archive_status;
 +
 +    while( !( archive_status = archive_read_next_header( p_arc, &entry ) ) )
 +    {
 +        if( archive_entry_filetype( entry ) == AE_IFDIR )
 +            continue;
 +
 +        char const* path = archive_entry_pathname( entry );
 +        char*       mrl  = vlc_stream_extractor_CreateMRL( p_extractor, path );
 +
 +        if( unlikely( !mrl ) )
 +            return VLC_ENOMEM;
 +
 +        input_item_t* p_item = input_item_New( mrl, path );
 +
 +        free( mrl );
 +
 +        if( unlikely( !p_item ) )
 +            return VLC_ENOMEM;
 +
 +
 +        input_item_CopyOptions( p_node->p_item, p_item );
 +        input_item_node_AppendItem( p_node, p_item );
 +        input_item_Release( p_item );
 +
 +        if( archive_read_data_skip( p_arc ) )
 +            break;
 +    }
 +
 +    if( archive_status != ARCHIVE_EOF )
 +        return VLC_EGENERIC;
 +
 +    return VLC_SUCCESS;
 +}
 +
 +static ssize_t Read( stream_extractor_t *p_extractor, void* p_data, size_t i_size )
 +{
 +    char dummy_buffer[ 1024 ];
 +
 +    private_sys_t* p_sys = p_extractor->p_sys;
 +    libarchive_t* p_arc = p_sys->p_archive;
 +    ssize_t       i_ret;
 +
 +    if( !p_sys->p_entry )
 +        return 0;
 +
 +    i_ret = archive_read_data( p_arc,
 +      p_data ? p_data :                        dummy_buffer,
 +      p_data ? i_size : __MIN( i_size, sizeof( dummy_buffer ) ) );
 +
 +    switch( i_ret )
 +    {
 +        case ARCHIVE_WARN:
 +        case ARCHIVE_RETRY:
 +        case ARCHIVE_FAILED:
 +        case ARCHIVE_FATAL:
 +            msg_Err( p_extractor, "libarchive: %s", archive_error_string( p_arc ) );
 +            return 0;
 +    }
 +
 +    p_sys->i_offset += i_ret;
 +    return i_ret;
 +}
 +
 +static int Seek( stream_extractor_t* p_extractor, uint64_t i_req )
 +{
 +    private_sys_t* p_sys = p_extractor->p_sys;
 +
 +    if( !p_sys->p_entry )
 +        return VLC_EGENERIC;
 +
 +    if( !p_sys->b_seekable_source )
 +        return VLC_EGENERIC;
 +
 +    if( !p_sys->b_seekable_archive
 +      || archive_seek_data( p_sys->p_archive, i_req, SEEK_SET ) < 0 )
 +    {
 +        msg_Dbg( p_extractor, "libarchive seek failed: '%s' (falling back to "
 +          "dumb seek)", archive_error_string( p_sys->p_archive ) );
 +
 +        uint64_t i_offset = p_sys->i_offset;
 +        uint64_t i_skip   = i_req - i_offset;
 +
 +        /* RECREATE LIBARCHIE HANDLE IF WE ARE SEEKING BACKWARDS */
 +
 +        if( i_req < i_offset )
 +        {
 +            if( archive_clean( p_extractor ) )
 +                goto error;
 +
 +            if( archive_init( p_extractor ) )
 +                goto error;
 +
 +            if( archive_seek_subentry( p_extractor, p_extractor->identifier ) )
 +                goto error;
 +
 +            i_skip   = i_req;
 +            i_offset = 0;
 +        }
 +
 +        /* SKIP _DECOMPRESSED_ DATA */
 +
 +        while( i_skip )
 +        {
 +            ssize_t i_read = Read( p_extractor, NULL, i_skip );
 +
 +            if( 1 > i_read )
 +                goto error;
 +
 +            i_offset += i_read;
 +            i_skip   -= i_read;
 +        }
 +    }
 +
 +    p_sys->i_offset = i_req;
 +    return VLC_SUCCESS;
 +
 +error:
 +    return VLC_EGENERIC;
 +}
 +
 +static void Close( vlc_object_t* p_obj )
 +{
 +    stream_extractor_t*  p_extractor = (stream_extractor_t*)p_obj;
 +    private_sys_t* p_sys = p_extractor->p_sys;
 +
 +    archive_clean( p_extractor );
 +
 +    for( size_t i = 0; i < p_sys->i_callback_data; ++i )
 +        free( p_sys->pp_callback_data[i] );
 +
 +    free( p_sys->psz_additional_files );
 +    free( p_sys->pp_callback_data );
 +    free( p_sys );
 +}
 +
 +/* ------------------------------------------------------------------------- */
 +
 +static int probe( stream_extractor_t* p_extractor )
 +{
 +    /* TODO: rewrite in a cleaner manner */
 +
 +    struct
 +    {
 +        const uint16_t i_offset;
 +        const uint8_t  i_length;
 +        const char * const p_bytes;
 +    } const magicbytes[] = {
 +        /* keep heaviest at top */
 +        { 257, 5, "ustar" },              //TAR
 +        { 0,   7, "Rar!\x1A\x07" },       //RAR
 +        { 0,   6, "7z\xBC\xAF\x27\x1C" }, //7z
 +        { 0,   4, "xar!" },               //XAR
 +        { 0,   4, "PK\x03\x04" },         //ZIP
 +        { 0,   4, "PK\x05\x06" },         //ZIP
 +        { 0,   4, "PK\x07\x08" },         //ZIP
 +        { 2,   3, "-lh" },                //LHA/LHZ
 +        { 0,   3, "\x1f\x8b\x08" },       // Gzip
 +        { 0,   3, "PAX" },                //PAX
 +        { 0,   6, "070707" },             //CPIO
 +        { 0,   6, "070701" },             //CPIO
 +        { 0,   6, "070702" },             //CPIO
 +        { 0,   4, "MSCH" },               //CAB
 +    };
 +
 +    const uint8_t *p_peek;
 +
 +    int i_peek = vlc_stream_Peek( p_extractor->source, &p_peek,
 +      magicbytes[0].i_offset + magicbytes[0].i_length);
 +
 +    for(unsigned i=0; i < ARRAY_SIZE( magicbytes ); i++)
 +    {
 +        if (i_peek < magicbytes[i].i_offset + magicbytes[i].i_length)
 +            continue;
 +
 +        if ( !memcmp(p_peek + magicbytes[i].i_offset,
 +            magicbytes[i].p_bytes, magicbytes[i].i_length) )
 +            return VLC_SUCCESS;
 +    }
 +
 +    return VLC_EGENERIC;
 +}
 +
 +static int setup( stream_extractor_t* p_extractor, char* psz_files )</code></pre>
</blockquote>
<pre><code> This seems to assume psz_files is an allocated string, while you duplicate it below. I'd
 either remove the duplication or mark psz_files as a const char*.</code></pre>
</blockquote>
<p>The duplication is because the usage of <code>strtok</code>, and given that <code>setup</code> has internal linkage I considered it to be okay to have <code>psz_files</code> declared as <code>char*</code>, and that it is assumed to refer to a dynamically allocated entity.</p>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> +{
 +    private_sys_t* p_sys  = calloc( 1, sizeof( *p_sys ) );
 +    char* psz_data = NULL;
 +
 +    if( unlikely( !p_sys ) ) goto error;
 +    else                     p_sys->p_extractor = p_extractor;</code></pre>
</blockquote>
<pre><code> I'd remove the else clause</code></pre>
</blockquote>
<p>It’s hard to be one of those developers with a very perculiar taste when it comes to code-writing, and what one considers “easily readable”; but I will change it, hehe!</p>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> +
 +    if( archive_push_resource( p_sys, p_extractor->source, NULL ) )
 +        goto error;
 +
 +    if( psz_files )
 +    {
 +        char* psz_data = strdup( psz_files );</code></pre>
</blockquote>
<pre><code> You probably don't want to shadow the previous psz_data declaration</code></pre>
</blockquote>
<p>Correct, not sure why I didn’t remove that (and fix the below mentioned thing <code>(A)</code>) during the last refactor.</p>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> +
 +        if( unlikely( !psz_data ) )
 +            goto error;
 +
 +        for( char* state,
 +                 * path = strtok_r( psz_data, ",", &state );
 +             path; path = strtok_r(     NULL, ",", &state ) )
 +        {
 +            if( path == psz_data )
 +                continue;
 +
 +            if( archive_push_resource( p_sys, NULL, path ) )</code></pre>
</blockquote>
<pre><code> It seems dubious to pass "path" here and let the archive_push_resource take ownership of
 it.</code></pre>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> +                goto error;
 +        }
 +    }
 +
 +    p_sys->psz_additional_files = psz_data;</code></pre>
</blockquote>
</blockquote>
<p><code>(A)</code>: the above should be <code>psz_files</code>, and not <code>psz_data</code>.. though it is important to note that <code>private_sys_t::psz_additional_files</code> is not used anymore, and as such everything regarding that variable should be removed.</p>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> +    p_extractor->p_sys = p_sys;
 +
 +    return VLC_SUCCESS;
 +
 +error:
 +    free( psz_data );
 +    free( p_sys );
 +
 +    return VLC_EGENERIC;
 +}
 +
 +static int archive_init( stream_extractor_t* p_extractor )
 +{
 +    private_sys_t* p_sys = p_extractor->p_sys;
 +
 +    /* CREATE ARCHIVE HANDLE */
 +
 +    p_sys->p_archive = archive_read_new();
 +
 +    if( unlikely( !p_sys->p_archive ) )
 +    {
 +        msg_Dbg( p_extractor, "unable to create libarchive handle" );
 +
 +        goto error;
 +    }
 +
 +    /* SETUP SEEKING */
 +
 +    p_sys->b_seekable_archive = false;
 +
 +    if( vlc_stream_Control( p_extractor->source, STREAM_CAN_SEEK,
 +        &p_sys->b_seekable_source ) )
 +    {
 +        msg_Warn( p_extractor, "unable to query whether source stream can seek" );
 +        p_sys->b_seekable_source = false;</code></pre>
</blockquote>
</blockquote>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> I'm not sure if the new psz_data is intented, but you'd have
 multiple pieces of the same allocated string being seen as
 individual strings from archive_push_resource, or</code></pre>
</blockquote>
<p>I guess this remark was intended at an earlier place in your reply, and it seem to have ended up on an unrelated line while also being on the same line as the diff itself.</p>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> +    }
 +
 +    if( p_sys->b_seekable_source )
 +    {
 +        if( archive_read_set_seek_callback( p_sys->p_archive,
 +            libarchive_seek_cb ) )
 +        {
 +            msg_Err( p_extractor, "archive_read_set_callback failed, aborting." );
 +            goto error;
 +        }
 +    }
 +
 +    /* ENABLE ALL FORMATS/FILTERS */
 +
 +    archive_read_support_filter_all( p_sys->p_archive );
 +    archive_read_support_format_all( p_sys->p_archive );
 +
 +    /* REGISTER CALLBACK DATA */
 +
 +    if( archive_read_set_switch_callback( p_sys->p_archive,
 +        libarchive_jump_cb ) )
 +    {
 +        msg_Err( p_extractor, "archive_read_set_switch_callback failed, aborting." );
 +        goto error;
 +    }
 +
 +    for( size_t i = 0; i < p_sys->i_callback_data; ++i )
 +    {
 +        if( archive_read_append_callback_data( p_sys->p_archive,
 +            p_sys->pp_callback_data[i] ) )
 +        {
 +            goto error;
 +        }
 +    }
 +
 +    /* OPEN THE ARCHIVE */
 +
 +    if( archive_read_open2( p_sys->p_archive, p_sys->pp_callback_data[0], NULL,
 +        libarchive_read_cb, libarchive_skip_cb, libarchive_exit_cb ) )
 +    {
 +        msg_Dbg( p_extractor, "libarchive: %s",
 +          archive_error_string( p_sys->p_archive ) );
 +
 +        goto error;
 +    }
 +
 +    return VLC_SUCCESS;
 +
 +error:
 +    return VLC_EGENERIC;</code></pre>
</blockquote>
<pre><code> Don't you miss some resource releasing before returning? Otherwise I guess you could
 return directly.</code></pre>
</blockquote>
<p>The clean-up happens in <code>Close</code> which is called from <code>Open</code> if <code>archive_init</code> fails, the reason for the usage of <code>goto</code> instead of returning directly is because I personally find it easier to read (but I will return directly after the fixes of other things you have addressed).</p>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> +}
 +
 +static int archive_clean( stream_extractor_t* p_extractor )
 +{
 +    private_sys_t* p_sys = p_extractor->p_sys;
 +    libarchive_t* p_arc = p_sys->p_archive;
 +
 +    archive_entry_free( p_sys->p_entry );
 +    archive_read_free( p_arc );
 +
 +    p_sys->p_entry   = NULL;
 +    p_sys->p_archive = NULL;
 +
 +    return VLC_SUCCESS;
 +}
 +
 +int archive_seek_subentry( stream_extractor_t* p_extractor, char const* psz_subentry )
 +{
 +    private_sys_t* p_sys = p_extractor->p_sys;
 +    libarchive_t* p_arc = p_sys->p_archive;
 +
 +    struct archive_entry* entry;
 +    int archive_status;
 +
 +    while( !( archive_status = archive_read_next_header( p_arc, &entry ) ) )
 +    {
 +        char const* entry_path = archive_entry_pathname( entry );
 +
 +        if( strcmp( entry_path, psz_subentry ) == 0 )
 +        {
 +            p_sys->p_entry = archive_entry_clone( entry );
 +
 +            if( unlikely( !p_sys->p_entry ) )
 +                return VLC_ENOMEM;
 +
 +            break;
 +        }
 +
 +        archive_read_data_skip( p_arc );
 +    }
 +
 +    switch( archive_status )
 +    {
 +        case ARCHIVE_WARN:
 +            msg_Warn( p_extractor,
 +              "libarchive: %s", archive_error_string( p_arc ) );
 +
 +        case ARCHIVE_EOF:
 +        case ARCHIVE_FATAL:
 +        case ARCHIVE_RETRY:
 +            archive_set_error( p_arc, ARCHIVE_FATAL,
 +                "archive does not contain >>> %s <<<", psz_subentry );
 +
 +            return VLC_EGENERIC;
 +    }
 +
 +    /* check if seeking is supported */
 +
 +    if( p_sys->b_seekable_source )
 +    {
 +        if( archive_seek_data( p_sys->p_archive, 0, SEEK_CUR ) >= 0 )
 +            p_sys->b_seekable_archive = true;
 +    }
 +
 +    return VLC_SUCCESS;
 +}
 +
 +static int archive_push_resource( private_sys_t* p_sys,
 +  stream_t* p_source, char* psz_url )
 +{
 +    libarchive_callback_t** pp_callback_data;
 +    libarchive_callback_t*   p_callback_data;
 +
 +    /* INCREASE BUFFER SIZE */
 +
 +    pp_callback_data = realloc( p_sys->pp_callback_data,
 +      sizeof( *p_sys->pp_callback_data ) * ( p_sys->i_callback_data + 1 ) );
 +
 +    if( unlikely( !pp_callback_data ) )
 +        return VLC_ENOMEM;
 +
 +    /* CREATE NEW NODE */
 +
 +    p_callback_data = malloc( sizeof( *p_callback_data ) );
 +
 +    if( unlikely( !p_callback_data ) )
 +    {
 +        free( pp_callback_data );
 +        return VLC_ENOMEM;
 +    }
 +
 +    /* INITIALIZE AND APPEND */
 +
 +    p_callback_data->p_source = p_source;
 +    p_callback_data->psz_url  = psz_url;
 +    p_callback_data->p_sys    = p_sys;
 +
 +    pp_callback_data[ p_sys->i_callback_data++ ] = p_callback_data;
 +    p_sys->pp_callback_data = pp_callback_data;
 +
 +    return VLC_SUCCESS;
 +}
 +
 +
 +int libarchive_jump_cb( libarchive_t* p_arc, void* p_obj_current,
 +  void* p_obj_next )
 +{
 +    libarchive_callback_t* p_current = (libarchive_callback_t*)p_obj_current;
 +    libarchive_callback_t* p_next    = (libarchive_callback_t*)p_obj_next;
 +
 +    libarchive_exit_cb( p_arc, p_current );
 +
 +    if( p_next->p_source == NULL )
 +        p_next->p_source = vlc_stream_NewURL( p_next->p_sys->p_extractor,
 +                             p_next->psz_url );
 +
 +    return p_next->p_source ? ARCHIVE_OK : ARCHIVE_FATAL;
 +}
 +
 +int libarchive_exit_cb( libarchive_t* p_arc, void* p_obj )
 +{
 +    VLC_UNUSED( p_arc );
 +
 +    libarchive_callback_t* p_cb = (libarchive_callback_t*)p_obj;
 +
 +    if( p_cb->p_sys->p_extractor->source == p_cb->p_source )
 +    {
 +        /* do not close our mother stream */
 +        if( vlc_stream_Seek( p_cb->p_source, 0 ) )
 +            return ARCHIVE_FATAL;
 +    }
 +    else if( p_cb->p_source )
 +    {
 +        vlc_stream_Delete( p_cb->p_source );
 +        p_cb->p_source = NULL;
 +    }
 +
 +    return ARCHIVE_OK;
 +}
 +
 +la_int64_t libarchive_skip_cb( libarchive_t* p_arc, void* p_obj,
 +  off_t i_request )
 +{
 +    VLC_UNUSED( p_arc );
 +
 +    libarchive_callback_t* p_cb = (libarchive_callback_t*)p_obj;
 +
 +    stream_t*  p_source = p_cb->p_source;
 +    private_sys_t* p_sys = p_cb->p_sys;
 +
 +    /* TODO: fix b_seekable_source on libarchive_callback_t */
 +
 +    if( p_sys->b_seekable_source )
 +    {
 +        if( vlc_stream_Seek( p_source, vlc_stream_Tell( p_source ) + i_request ) )
 +            return ARCHIVE_FATAL;
 +
 +        return i_request;
 +    }
 +
 +    ssize_t i_read = vlc_stream_Read( p_source, NULL, i_request );
 +    return  i_read >= 0 ? i_read : ARCHIVE_FATAL;
 +}
 +
 +la_int64_t libarchive_seek_cb( libarchive_t* p_arc, void* p_obj,
 +  la_int64_t offset, int whence )
 +{
 +    VLC_UNUSED( p_arc );
 +
 +    libarchive_callback_t* p_cb = (libarchive_callback_t*)p_obj;
 +    stream_t* p_source = p_cb->p_source;
 +
 +    ssize_t whence_pos;
 +
 +    switch( whence )
 +    {
 +        case SEEK_SET: whence_pos = 0;                           break;
 +        case SEEK_CUR: whence_pos = vlc_stream_Tell( p_source ); break;
 +        case SEEK_END: whence_pos = stream_Size( p_source ) - 1; break;
 +              default: vlc_assert_unreachable();
 +
 +    }
 +
 +    if( whence_pos < 0 || vlc_stream_Seek( p_source, whence_pos + offset ) )
 +        return ARCHIVE_FATAL;
 +
 +    return vlc_stream_Tell( p_source );
 +}
 +
 +la_ssize_t libarchive_read_cb( libarchive_t* p_arc, void* p_obj,
 +  const void** pp_dst )
 +{
 +    VLC_UNUSED( p_arc );
 +
 +    libarchive_callback_t* p_cb = (libarchive_callback_t*)p_obj;
 +
 +    stream_t*  p_source = p_cb->p_source;
 +    private_sys_t* p_sys = p_cb->p_sys;
 +
 +    ssize_t i_ret = vlc_stream_Read( p_source, &p_sys->buffer,
 +      sizeof( p_sys->buffer ) );
 +
 +    if( i_ret < 0 )
 +    {
 +        archive_set_error( p_sys->p_archive, ARCHIVE_FATAL,
 +          "libarchive_read_cb failed = %" PRId64, i_ret );
 +
 +        return ARCHIVE_FATAL;
 +    }
 +
 +    *pp_dst = &p_sys->buffer;
 +
 +    return i_ret;
 +}
 diff --git a/po/POTFILES.in b/po/POTFILES.in
 index 68010c715e..964b56d676 100644
 --- a/po/POTFILES.in
 +++ b/po/POTFILES.in
 @@ -1017,6 +1017,7 @@ modules/spu/mosaic.c
  modules/spu/remoteosd.c
  modules/spu/rss.c
  modules/spu/subsdelay.c
 +modules/stream_extractor/archive.c
  modules/stream_filter/adf.c
  modules/stream_filter/aribcam.c
  modules/stream_filter/cache_block.c
 </code></pre>
</blockquote>
</blockquote>
</body>
</html>