doug-gilbert / sg3_utils

Author's own git mirror of his sg3_utils subversion repository. Note: default branch is now _main_. It includes tags from the various releases.
https://sg.danny.cz/sg/sg3_utils.html
Other
25 stars 22 forks source link

mmap IO example mistake #53

Open GrigorenkoPV opened 2 months ago

GrigorenkoPV commented 2 months ago
diff --git a/examples/sg_simple4.c b/examples/sg_simple4.c
index 95a5023b..6f3799ad 100644
--- a/examples/sg_simple4.c
+++ b/examples/sg_simple4.c
@@ -118,7 +118,7 @@ int main(int argc, char * argv[])
     io_hdr.mx_sb_len = sizeof(sense_buffer);
     io_hdr.dxfer_direction = SG_DXFER_FROM_DEV;
     io_hdr.dxfer_len = INQ_REPLY_LEN;
-    /* io_hdr.dxferp = inqBuff; // ignored in mmap-ed IO */
+    io_hdr.dxferp = inqBuff;
     io_hdr.cmdp = inq_cdb;
     io_hdr.sbp = sense_buffer;
     io_hdr.timeout = 20000;     /* 20000 millisecs == 20 seconds */
mwilck commented 2 months ago

This looks correct, but some comments would be appreciated. This code is ancient (2007).

Did you run the test program, and got a SEGV or similar error? Was the error gone after this change?

GrigorenkoPV commented 2 months ago

Without this change, ioctl returns -1 aka EFAULT (Bad address).

With this change, it works.

But since now it attempts to write to this address, you also have to mmap with | PROT_WRITE, which would explain why this comment is lying https://github.com/doug-gilbert/sg3_utils/blob/2355dc4b451989291df695148cd8d8d03b3d987e/examples/sg_simple4.c#L98-L100

All of this stuff is weird.

GrigorenkoPV commented 2 months ago

You can also do this

diff --git a/examples/sg_simple4.c b/examples/sg_simple4.c
index 95a5023b..31c31c84 100644
--- a/examples/sg_simple4.c
+++ b/examples/sg_simple4.c
@@ -53,6 +53,7 @@ int main(int argc, char * argv[])
                                 {0x00, 0, 0, 0, 0, 0};
     unsigned char * inqBuff;
     unsigned char * inqBuff2;
+    unsigned char bufff[INQ_REPLY_LEN];
     sg_io_hdr_t io_hdr;
     char * file_name = 0;
     char ebuff[EBUFF_SZ];
@@ -118,7 +119,7 @@ int main(int argc, char * argv[])
     io_hdr.mx_sb_len = sizeof(sense_buffer);
     io_hdr.dxfer_direction = SG_DXFER_FROM_DEV;
     io_hdr.dxfer_len = INQ_REPLY_LEN;
-    /* io_hdr.dxferp = inqBuff; // ignored in mmap-ed IO */
+    io_hdr.dxferp = bufff;
     io_hdr.cmdp = inq_cdb;
     io_hdr.sbp = sense_buffer;
     io_hdr.timeout = 20000;     /* 20000 millisecs == 20 seconds */
@@ -148,7 +149,7 @@ int main(int argc, char * argv[])
     }

     if (ok) { /* output result if it is available */
-        char * p = (char *)inqBuff;
+        char * p = (char *)bufff;
         int f = (int)*(p + 7);
         printf("Some of the INQUIRY command's results:\n");
         printf("    %.8s  %.16s  %.4s  ", p + 8, p + 16, p + 32);

And it works too. Maybe kernel just ignore SG_FLAG_MMAP_IO?

mwilck commented 2 months ago

I glanced at the kernel code, but couldn't figure it out at a quick glance. The flag not completely ignored, that's for sure. I've never seen any code using it so far.

@doug-gilbert, any idea?