Re: [Libcdio-devel] CD_MSF_FORMAT vs LBA on NetBSD

[Thomas Schmitt]

Hi,

> I've just finished a bug-finding session on cd-paranoia.

What repo are you working with ?
I'm now looking at
  https://github.com/rocky/libcdio-paranoia/tree/master

------------------------------------------------------------------

> diff --git a/lib/cdda_interface/cddap_interface.c
> -  return --i_track;  /* number of tracks returned does not include lead-out 
> */
> +  return d->tracks;  /* number of tracks returned does not include lead-out 
> */

Ok.


> -  for(i=1;i<=d->tracks;i++){
> +  for(i=first_track; i<first_track+d->tracks; i++){

Ok.

------------------------------------------------------------------

> --- a/lib/cdda_interface/toc.c
> ...
> -  for ( i=0; i<d->tracks; i++ )
> +  for ( i=first_track;i<first_track + d->tracks;i++ )

This one looks fishy. Default first_track is 1. To map this on 0 you need
to subtract 1.
Like:

  -  for(i=1;i<=d->tracks;i++){
  +  for(i=first_track - 1; i<first_track - 1 + d->tracks;i++ )


>     if( cdda_track_audiop(d, i+1)==1 ) {
> -      if (i == 0) /* disc starts at lba 0 if first track is an audio track
*/
> +      if (i == first_track) /* disc starts at lba 0 if first track is an

The same.
(Partly this compensates the lack of -1 above. But then you'd have to
 change "i+1" to "i".)

Proposal:

  -      if (i == 0) /* ... */
  +      if (i == first_track - 1) /* ... */

------------------------------------------------------------------

> +++ b/lib/paranoia/p_block.c
>    i = cdda_sector_gettrack(d, p->cursor);
> +  if( i==0 )
> +    i = cdio_get_first_track_num(d->p_cdio);

The reply 0 (LSN before first track) seems to be normal and is already
handled. I guess that actually this line needed fixing:

>      if ( 0 == i ) i++;

It seems to assume that the first track number is a (semi-) error code
plus 1. (Kids, don't do this at home.)

So my proposal is to merge the new "if" with the existing "if":

     if ( CDIO_INVALID_TRACK != i ) {
  -     if ( 0 == i ) i++;
  +     if ( 0 == i )
  +       i = cdio_get_first_track_num(d->p_cdio);

(But i concede that your fix should work and make "if ( 0 == i ) i++;"
 a useless piece of code. You could just throw it out.)

------------------------------------------------------------------

> +++ b/src/cd-paranoia.c
>        int track1 = cdda_sector_gettrack(d, i_first_lsn);
> +      if( track1==0 )
> +        track1 = cdio_get_first_track_num(d->p_cdio);
> +
>        int track2 = cdda_sector_gettrack(d, i_last_lsn);
>        long off1  = i_first_lsn - cdda_track_firstsector(d, track1);
>        long off2  = i_last_lsn  - cdda_track_firstsector(d, track2);

Hm. If track1 == 0 appears now, then it propbably appears normally.
(And what about track2 ?)

Function cdda_track_firstsector() in lib/cdda_interface/toc.c returns
with track number 0 either error or 0, depending on whether the first
track starts at LBA 0 or at LBA >0, respectively.
I.e. track number 0 addresses possible blocks before the first track.

I think that only the error check loop needs a skip feature from
pseudo-track number 0 to the first existing track number.
Like:

  -   for( i=track1; i<=track2; i++ )
  +   for( i=track1; i<=track2; i++ ) {
        if(i != 0 && !cdda_track_audiop(d,i)){
          report("Selected span contains non audio track at track %02d. 
Aborting.\n\n", i);
          exit(1);
        }
  +     if(i == 0)
  +       i = cdio_get_first_track_num(d->p_cdio) - 1;
  +   }

------------------------------------------------------------------

Have a nice day :)

Thomas