Rockbox mail archiveSubject: Re: id3.c: reading id3 tags, is this a bug?
Re: id3.c: reading id3 tags, is this a bug?
From: TP Diffenbach <rockbox_at_diffenbach.org>
Date: Tue, 18 Mar 2003 20:37:37 -0500
Examining the latest CVS with Linus Nielsen Feltzing's changes, I'm still not sure that the code is correct. Specifically, if the read fails (bytes read != bytes requested), I wonder why we shouldn't simply bail out. If we aren't bailing out, I suspect that saze should de decreased by bytesread in all cases; currently sometimes it's reduced by bytesread, sometimes by framelen.
One thing especially confuses me:
static int read_frame(int fd, unsigned char *buf, char **destptr, int framelen)
bytesread = read(fd, buf, framelen);
if(bytesread < 0)
return bytesread * 10 - 1;
A negative return from read, if I recall correctly, means that the read failed. read_frame's caller is only interested in whether read_frame's return value is less than zero. So why mutiply read's return value by ten, and subtract one? What is this useful for?
Incidently, my original question was motivated by a desire to make some changes to id3.c; rather than anyone else making any changes, I'd be glad to do it.
Quoting Linus Nielsen Feltzing <linus_at_haxx.se>:
> TP Diffenbach wrote:
> > However, size refers to the size of the frame being read from, in
> > mp3's id3 tag. It seems to me that even if unicode_munge is
> > the size of the copy of the string that we are saving, the size
> > remaining on disk in the mp3's id3 tag remains the same. If this
> > the case, size should be set /before/ bytesread is (possibly)
> > adjusted by unicode_munge:
> You are absolutely correct. Thanks for pointing that out.
> > Also, it seems that the call to read should be checked for error
> > EOF:
> I agree.
> > Thanks.-- Archos FM needs a Rockbox!
> Thank you for finding bugs in our code.
-- Archos FM needs a Rockbox!Received on 2003-03-19