[9front] [patch] cdfs handle block sizes correctly

[9front] [patch] cdfs handle block sizes correctly

[Arne Meyer]

The Readblock enum does not work when you try to read audio cds. 4 cdda blocks are lager than 8192 and the command fails (at least when using an usb cd drive). This sets the block count according to the track block size. With this and the fix for libdisk I can rip audio cds on my el cheapo usb dvd drive. Tested with "Midnight Oil - Blue Sky Mining"
Greetings,
Arne

diff e938acc8ff64a3cbbd6ef7ba88f83e3f03ede681 uncommitted
--- a/sys/src/cmd/cdfs/dat.h
+++ b/sys/src/cmd/cdfs/dat.h
@@ -133,10 +133,11 @@
        DVDNblock = 16,         /* DVD ECC block is 16 sectors */
        BDNblock = 32,          /* BD ECC block (`cluster') is 32 sectors */
        /*
-        * make a single transfer fit in a 9P rpc.  if we don't do this,
-        * remote access (e.g., via /mnt/term/dev/sd*) fails mysteriously.
+        * number of blocks read/written must fit in this. if we don't do this,
+        * remote access (e.g., via /mnt/term/dev/sd* or nusb/disk) fails mysteriously.
+        * see /sys/src/9/port/devmnt.c MAXRPC.
         */
-       Readblock = 8192/BScdrom,
+       Maxrpc = 8192,
 };

 typedef struct Buf Buf;
--- a/sys/src/cmd/cdfs/mmc.c
+++ b/sys/src/cmd/cdfs/mmc.c
@@ -1171,7 +1171,7 @@
        o->track = &drive->track[trackno];
        o->nchange = drive->nchange;
        o->omode = OREAD;
-       o->buf = bopen(mmcread, OREAD, o->track->bs, Readblock);
+       o->buf = bopen(mmcread, OREAD, o->track->bs, Maxrpc/o->track->bs);
        o->buf->otrack = o;

        aux->nropen++;
@@ -1395,7 +1395,7 @@
        o->nchange = drive->nchange;
        o->omode = OWRITE;
        o->track = t;
-       o->buf = bopen(mmcwrite, OWRITE, bs, Readblock);
+       o->buf = bopen(mmcwrite, OWRITE, bs, Maxrpc/bs);
        o->buf->otrack = o;

        aux->nwopen++;

Re: [9front] [patch] cdfs handle block sizes correctly

[ori]

Quoth Arne Meyer <meyer.arne83@netcologne.de>:
> The Readblock enum does not work when you try to read audio cds.  4
> cdda blocks are lager than 8192 and the command fails (at least when
> using an usb cd drive).  This sets the block count according to the
> track block size.  With this and the fix for libdisk I can rip audio
> cds on my el cheapo usb dvd drive.  Tested with "Midnight Oil - Blue
> Sky Mining"
> Greetings,
> Arne


> 
>  typedef struct Buf Buf;
> --- a/sys/src/cmd/cdfs/mmc.c
> +++ b/sys/src/cmd/cdfs/mmc.c
> @@ -1171,7 +1171,7 @@
>         o->track = &drive->track[trackno];
>         o->nchange = drive->nchange;
>         o->omode = OREAD;
> -       o->buf = bopen(mmcread, OREAD, o->track->bs, Readblock);
> +       o->buf = bopen(mmcread, OREAD, o->track->bs, Maxrpc/o->track->bs);

these changes smell funny to me; if the block size is larger than Maxrpc, we
set nblock to 0. Looking at bread in buf.c, it seems like we'd simplly end up
trying to read 0 bytes out of these large buffers.

Re: [9front] [patch] cdfs handle block sizes correctly

[Arne Meyer]

[-- Attachment #1: Type: text/plain, Size: 1513 bytes --]

The old Readblock value is fine for data tracks, because 4 2048 byte blocks fit in the 8192 byte RPCMAX. But cdda blocks are 2352 bytes and 4 of those don't fit into 8192 bytes and stuff breaks. If I read the code correctly my change should set the number of blocks to 4 for data tracks and to 3 for everything else.

I've attached the patch to this mail to preserve whitespace.

> ori@eigenstate.org hat am 01.10.2022 21:20 GMT geschrieben:
> 
>  
> Quoth Arne Meyer <meyer.arne83@netcologne.de>:
> > The Readblock enum does not work when you try to read audio cds.  4
> > cdda blocks are lager than 8192 and the command fails (at least when
> > using an usb cd drive).  This sets the block count according to the
> > track block size.  With this and the fix for libdisk I can rip audio
> > cds on my el cheapo usb dvd drive.  Tested with "Midnight Oil - Blue
> > Sky Mining"
> > Greetings,
> > Arne
> 
> 
> > 
> >  typedef struct Buf Buf;
> > --- a/sys/src/cmd/cdfs/mmc.c
> > +++ b/sys/src/cmd/cdfs/mmc.c
> > @@ -1171,7 +1171,7 @@
> >         o->track = &drive->track[trackno];
> >         o->nchange = drive->nchange;
> >         o->omode = OREAD;
> > -       o->buf = bopen(mmcread, OREAD, o->track->bs, Readblock);
> > +       o->buf = bopen(mmcread, OREAD, o->track->bs, Maxrpc/o->track->bs);
> 
> these changes smell funny to me; if the block size is larger than Maxrpc, we
> set nblock to 0. Looking at bread in buf.c, it seems like we'd simplly end up
> trying to read 0 bytes out of these large buffers.

[-- Attachment #2: cdfs.patch --]
[-- Type: application/octet-stream, Size: 1245 bytes --]

diff e938acc8ff64a3cbbd6ef7ba88f83e3f03ede681 uncommitted
--- a/sys/src/cmd/cdfs/dat.h
+++ b/sys/src/cmd/cdfs/dat.h
@@ -133,10 +133,11 @@
 	DVDNblock = 16,		/* DVD ECC block is 16 sectors */
 	BDNblock = 32,		/* BD ECC block (`cluster') is 32 sectors */
 	/*
-	 * make a single transfer fit in a 9P rpc.  if we don't do this,
-	 * remote access (e.g., via /mnt/term/dev/sd*) fails mysteriously.
+	 * number of blocks read/written must fit in this. if we don't do this,
+	 * remote access (e.g., via /mnt/term/dev/sd* or nusb/disk) fails mysteriously.
+	 * see /sys/src/9/port/devmnt.c MAXRPC.
 	 */
-	Readblock = 8192/BScdrom,
+	Maxrpc = 8192,
 };
 
 typedef struct Buf Buf;
--- a/sys/src/cmd/cdfs/mmc.c
+++ b/sys/src/cmd/cdfs/mmc.c
@@ -1171,7 +1171,7 @@
 	o->track = &drive->track[trackno];
 	o->nchange = drive->nchange;
 	o->omode = OREAD;
-	o->buf = bopen(mmcread, OREAD, o->track->bs, Readblock);
+	o->buf = bopen(mmcread, OREAD, o->track->bs, Maxrpc/o->track->bs);
 	o->buf->otrack = o;
 
 	aux->nropen++;
@@ -1395,7 +1395,7 @@
 	o->nchange = drive->nchange;
 	o->omode = OWRITE;
 	o->track = t;
-	o->buf = bopen(mmcwrite, OWRITE, bs, Readblock);
+	o->buf = bopen(mmcwrite, OWRITE, bs, Maxrpc/bs);
 	o->buf->otrack = o;
 
 	aux->nwopen++;

Re: [9front] [patch] cdfs handle block sizes correctly

[ori]

Quoth Arne Meyer <meyer.arne83@netcologne.de>:
> The old Readblock value is fine for data tracks, because 4 2048 byte blocks fit in the 8192 byte RPCMAX. But cdda blocks are 2352 bytes and 4 of those don't fit into 8192 bytes and stuff breaks. If I read the code correctly my change should set the number of blocks to 4 for data tracks and to 3 for everything else.

Ah, I misunderstood you -- I thought you were saying you had disks
where the blocksize was larger than RPCMAX (8k).

I'll take another look.

Re: [9front] [patch] cdfs handle block sizes correctly

[Arne Meyer]

ping?

> ori@eigenstate.org hat am 02.10.2022 20:04 GMT geschrieben:
> 
>  
> Quoth Arne Meyer <meyer.arne83@netcologne.de>:
> > The old Readblock value is fine for data tracks, because 4 2048 byte blocks fit in the 8192 byte RPCMAX. But cdda blocks are 2352 bytes and 4 of those don't fit into 8192 bytes and stuff breaks. If I read the code correctly my change should set the number of blocks to 4 for data tracks and to 3 for everything else.
> 
> Ah, I misunderstood you -- I thought you were saying you had disks
> where the blocksize was larger than RPCMAX (8k).
> 
> I'll take another look.

Re: [9front] [patch] cdfs handle block sizes correctly

[ori]

Quoth Arne Meyer <meyer.arne83@netcologne.de>:
> ping?
> 
> > ori@eigenstate.org hat am 02.10.2022 20:04 GMT geschrieben:
> > 
> >  
> > Quoth Arne Meyer <meyer.arne83@netcologne.de>:
> > > The old Readblock value is fine for data tracks, because 4 2048 byte blocks fit in the 8192 byte RPCMAX. But cdda blocks are 2352 bytes and 4 of those don't fit into 8192 bytes and stuff breaks. If I read the code correctly my change should set the number of blocks to 4 for data tracks and to 3 for everything else.
> > 
> > Ah, I misunderstood you -- I thought you were saying you had disks
> > where the blocksize was larger than RPCMAX (8k).
> > 
> > I'll take another look.

ponged and applied; thanks!