[MPlayer-dev-eng] [patch] play and seek files inside multivolume uncompressed rar files

[Emiel Neggers]

Hello Reimar,

On Fri, 2009-12-18 at 20:04 +0100, Reimar Döffinger wrote:
> A first review, it is unlikely to have caught everything, and almost
> completely ignores cosmetics.

Alright. I fixed/changed most of it after Compn's friendly nudge.
Hopefully even with K&R style (I didn't spend much time on it, but
there's always hope ;). This also includes fixes from Diego's review. If
we're lucky, this new version even fixes more problems than it
introduces. I do have a few questions.

> > +    // do not handle any non-rar file
> > +    if(!filename || !rar_has_rar_filename(filename)) {
> 
> If the user requested this stream module, you should honor that request and
> try to something sensible even if the extension isn't .rar

What's the best way to tell if the user requested this stream module?

> > +        m_struct_free(&stream_opts,opts);
> > +        return STREAM_ERROR;
> 
> Do the freeing in one place, using goto if necessary. If you have more than
> one place where things are freed that basically guarantees memory leaks
> in the long term.

And here I thought goto's were evil. I added one now, though.

> > +        res += read(rar->fd, (char*)buf + res, count - res);
> 
> That's not going to do the right thing if read fails.
> It also won't do the right thing if "count - res" is still larger
> than the content of the next file (try creating rar archives with e.g.
> 1 kB per part and it should become obvious).

Indeed. I changed this part (rar_read) to a while loop, but did not do
any elaborate testing. It is probably a good idea to verify these
changes specifically.

> > +    res = o & 0xffffffff;
> 
> What??

Lol. I remember being confused about that part.. the problem was that
removing that line made things not work. Now I see why - it tried to do
lseek(res) instead of lseek(o), doh.

> > +    long int res = 0;
> > +    long int pos = lseek(rar->fd, 0L, SEEK_CUR);
> 
> Simple rule: "long int" is always wrong.
> off_t is the right type, though int64_t is ok as well.

I changed several variables to off_t. There are no more long int's left.
But what should rar_size() return? off_t, size_t, int64_t, uint64_t?

- Emiel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: mplayer.svn.r30131-rar.20091228.2314.patch
Type: text/x-patch
Size: 22662 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/mplayer-dev-eng/attachments/20091228/efd63af2/attachment.bin>