Re: cdrom ioctls

In article <[email protected]> you wrote:
>   I have one problem with netblock - some cdrom ioctls have not right

(enbd only returns 0 for remote ioctl success, never a positive number)

I'd like to try a possible workaround. I attach a patch for the kernel
module. At your level, the idea is that you change the declaration of
your ioctl from

-   { CDROM_DRIVE_STATUS, _NEW_IOW(CDROM_DRIVE_STATUS, int), },

in the standard table to

+    { _NEW_IOWRS(CDROM_DRIVE_STATUS), NULL, cdrom_drive_status_size_user, cdrom_drive_status_copy_from_user, cdrom_drive_status_copy_to_user,},

in the special table in the table.h file.

Here the mentioned functions will be defined to make things work out as
you want. The designation  "WRS" means write/read/SPECIAL. Fortunately,
when I set up the remote ioctl mechanism, I included a means for doing
special things outside of the standard modes. Now is the time for a
first workout! Other changes are in support.

Firstly, in the indirect_ioctl_store() function which takes data from
the kernel to userspace for the ioctl, change the return value to reflect
exactly an _excess_ value over the transferred number of bytes that
is returned by the ioctl modules cp_to_user method, rather than always
returning just the transferred number (on success).

This is now going to be the eventual return value from the ioctl, and we
can now, with this change, define it to be what we want using the
cp_to_user method given in the IOWRS line mentioned above.

The only test on the returned value here was for -ve (= failure), so we
can afford to return any positive value on success, with no effects.

That's the first hunk below.

The second hunk is precisely the change to do_enbd_remote_ioctl to
transmitting exactly the value reported by indirect_ioctl_store as the
return value, instead of correcting it to be exactly zero if it was
positive.


diff -ur nbd-2.4.32-orig/kernel/linux-2.6.x/drivers/block/enbd/enbd_base.c nbd-2.4.32/kernel/linux-2.6.x/drivers/block/enbd/enbd_base.c
--- nbd-2.4.32-orig/kernel/linux-2.6.x/drivers/block/enbd/enbd_base.c	Sun Jul 31 20:28:43 2005
+++ nbd-2.4.32/kernel/linux-2.6.x/drivers/block/enbd/enbd_base.c	Sun Aug 21 16:59:34 2005
@@ -1371,7 +1371,8 @@
                 ENBD_ERROR("failed (%d) copy to user of %d bytes\n", err, size);
 		return -EFAULT;
         }
-	return size;
+        // PTB report strictly nonnegative for success, no offset
+	return err - size;
 }
 
 
@@ -3716,7 +3719,7 @@
                         kfree(buffer);
                 }
         }
-        err = ioctl_info->errors <= 0 ? ioctl_info->errors : -EINVAL;
+        err = ioctl_info->errors;
 
         // PTB Apparently put_req is done in the commit or error that released
         // the wait_on for rendezvous.  All we have left is the ioctl info.


The next change is in the enbd_ioctl module, to allow the
default ordinary copy_FROM_user (this time) method to return a
positive/negative value instead of something more ambiguous. This is
just cleanup.

diff -ur nbd-2.4.32-orig/kernel/linux-2.6.x/drivers/block/enbd/enbd_ioctl.c nbd-2.4.32/kernel/linux-2.6.x/drivers/block/enbd/enbd_ioctl.c
--- nbd-2.4.32-orig/kernel/linux-2.6.x/drivers/block/enbd/enbd_ioctl.c	Sat Jan  8 19:02:24 2005
+++ nbd-2.4.32/kernel/linux-2.6.x/drivers/block/enbd/enbd_ioctl.c	Sun Aug 21 13:33:50 2005
@@ -153,15 +186,23 @@
 
 
 
+/*
+ * copy from the userspace address @arg supplied to a kernel buffer @buf
+ * (IOWR)
+ */
 static int
 enbd_ioctl_copy_from_user (int cmd, char *buf, char *arg, int size)
 {
 
 	if (_IOC_SIZE (cmd) == _IOC_SIZEMASK) {
+
                 struct ioctl_special *special = ioctl_special_lookup_new(cmd);
+                int rem; // remainder not copied
+
                 if (!special)
                         return -1;
-	        return special->ioctl_copy_from_user (buf, arg, size);
+	        rem = special->ioctl_copy_from_user (buf, arg, size);
+		return size - rem;
 	}
 
 	if (_IOC_DIR (cmd) & _IOC_READ) {


Now for changes to the tables. The first hunk changes the main table
entry to make it point to a "special", and the second hunk defines the
functions referenced in the third hunk, which is the special table
entry addition.

The functions respectively tell the kernel to make an 8-byte buffer for
the ioctl (if I am wrong about the size, change it - it's whatever is
read in through the ioctl plus the 4-byte return value), tell the
kernel to load the write half of the buffer with the int value provided
by userspace, and tell the kernel to do  nothing when reading back from
the read half of the buffer except to lie and and add te value it finds
in that half to the total transferred bytes reported.

This is is likely to result in an  ioctl of the form

   file->ioctl(file, CDROM_DRIVE_STATUS, dummy)

arriving at the server (in fileserver.c), where dummy is int[2] with
dummy[0] containing the write value, and dummy[1] being the place to
put the required return value. It will require an intercept there. One
will need to add a

    case CDROM_DRIVE_STATUS:

in the fileserver.c ioctlfile() code.

    it should do a

      
      res = file->ioctl(file, CDROM_DRIVE_STATUS, ((int *)arg)[0])
      ((int *)arg)[1] = res;

I'll leave yu to experiment with that - it's in userspace, and it
should not die. I find it hard to visualize the whole sequence.


diff -ur nbd-2.4.32-orig/kernel/linux-2.6.x/include/linux/enbd_ioctl_table.h nbd-2.4.32/kernel/linux-2.6.x/include/linux/enbd_ioctl_table.h
--- nbd-2.4.32-orig/kernel/linux-2.6.x/include/linux/enbd_ioctl_table.h	Thu Jul 28 17:54:34 2005
+++ nbd-2.4.32/kernel/linux-2.6.x/include/linux/enbd_ioctl_table.h	Sun Aug 21 13:44:33 2005
@@ -278,9 +278,9 @@
 #ifdef CDROM_MEDIA_CHANGED
    { CDROM_MEDIA_CHANGED, _NEW_IOW(CDROM_MEDIA_CHANGED, int), },
 #endif /* CDROM_MEDIA_CHANGED */
 #ifdef CDROM_DRIVE_STATUS
-   { CDROM_DRIVE_STATUS, _NEW_IOW(CDROM_DRIVE_STATUS, int), },
+   { CDROM_DRIVE_STATUS, _NEW_IOWRS(CDROM_DRIVE_STATUS), },
 #endif /* CDROM_DRIVE_STATUS */
 #ifdef CDROM_DISC_STATUS
    { CDROM_DISC_STATUS, _NEW_IO(CDROM_DISC_STATUS), },
 #endif /* CDROM_DISC_STATUS */
@@ -681,6 +682,20 @@
    {0 , 0, },
 };
 
+    static int cdrom_drive_status_size_user(char *arg) {
+        // how big a kernel buffer to make to match user size
+        return sizeof(int) + sizeof(int);
+    }
+    static int cdrom_drive_status_copy_from_user(char *buf, char *arg, int size) {
+        // instead of indirect read, load the "address" itself (realy an int).
+        ((int *)buf)[0] = (int)arg;
+        return 0;
+    }
+    static int cdrom_drive_status_copy_to_user(char *arg, char *buf, int size) {
+        // the size is subtracted on reception to give the result
+        return size + ((int *)buf)[1];
+    }
+
 /*
  * This should be the table of special methods for certain ioctls.
  * The "new" code is the real index. It will have a size count of
@@ -689,6 +704,7 @@
  */
 static struct ioctl_special ioctl_special_tab[] = {
     // PTB last entry must be all zeros
+    { _NEW_IOWRS(CDROM_DRIVE_STATUS), NULL, cdrom_drive_status_size_user, cdrom_drive_status_copy_from_user, cdrom_drive_status_copy_to_user,},
     { 0, NULL, NULL, NULL, NULL, },
 };