Re: [linux-minidisc] another way to send SCSI commands

[Michael Karcher <Michael.Karcher@fu-berlin.de>]

[Cc to the dvd+rw-tools again, as another patch is attached. BTW: Thanks
for that lean code for that many OSsed!
Andy: Please do not misunderstand this mail as trying to make your code
look bad, even if I claim you violated some coding standard. I really
appreciate your work, and the code you have written shows no problems if
used correctly - it's just about prevention of abuse. This is of course
something one doesn't have in mind if one writes a SCSI transfer toolkit
for a single application instead of a general library. It's at least
partly our "fault" to take code out of dvd+rw-tools and use it
carelessly making our applications break.]

Am Samstag, den 04.12.2010, 14:33 +0100 schrieb Thomas Arp:
> I´m rewriting himdscsitest.c using the dvd-rw-tools headers for
> testing.
That's great!
> As far as i have tested yet when cmd.transport() has finished the
> Scsi_Command structure will be closed automatically.
As far as I can see, this is neither the case for Windows nor for Linux.
Can you point out the code you think it closes the file descriptor? I
see closing only in the destructor of Scsi_Command, so the device is
closed only when the Scsi_Command structure goes out of scope.

But I *do* see a potential problem that causes early closes: The
Scsi_Command structure must never be copied, as it violates the C++
"rule of three":
 http://en.wikipedia.org/wiki/Rule_of_three_%28C%2B%2B_programming%29
"must never be copied" is *not* talking about DRM here (SCNR), but means
that you can not pass a "Scsi_Command" as value to a subfunction,
because pass-by-value copies the data (and that includes the handle) to
local storage of the subfunction, and if that subfunction terminates,
the local copy is destructed, closing the handle, although the object it
was copied from in the caller still is alive. Also hell will break loose
(i.e. you will get early closed fds/double-closed fds/leaked fds) if you
assign one Scsi_Command object to another one. So do only pass the
Scsi_Command structure by reference or pointer!

> Should we disable this autoclose feature and use genaral open and
> close commands instead or should every send-command function have it´s
> own Scsi_Command structure, so that we only need to give the
> devicename as parameter instead of a Scsi_Command structure?
Of course we don't want an autoclose feature, but I am very confident
that what you observed was caused by copying Scsi_Command objects and
using the object as intended wouldn't cause problems. I attach a patch
to this mail that explicitly prohibits copying and assigment of
Scsi_Command objects, so with the patched transport.hxx your program
should fail to compile if it contains the problem I suppose.

> Thomas
Regards,
  Michael Karcher

PS: Don't worry about Qt widget/dialog box classes you write violating
the rule-of-three the same way. QObject itself is noncopyable (by
defining an inaccessible copy-assignment operator and copy-constructor),
so there already is an inherited user-defined copy constructor and
assignment operator preventing problems. Cleanup in a destructor doesn't
matter in this case.

PS2: Of course "Scsi_Command_Is_Noncopyable" is reinventing the wheel.
Boost already has boost::noncopyable and Qt has Q_DISABLY_COPY (although
just for internal use) and thousands of other people wrote these 6 lines
of code, too. But I don't want to pull boost into transport.hxx, as
everyone who needed to install boost on Windows systems surely can
understand. If our project already used boost, it would be a different
story, of course.

--- ../dvd+rw-tools-7.1/transport.hxx	2010-12-03 02:56:36.000000000 +0100
+++ transport.hxx	2010-12-04 18:17:57.000000000 +0100
@@ -136,6 +136,14 @@
 
   return plusminus;
 }
+
+class Scsi_Command_Is_Noncopyable {
+    /* declare a private copy constructor and assignment operator */
+    Scsi_Command_Is_Noncopyable(const Scsi_Command_Is_Noncopyable &);
+    Scsi_Command_Is_Noncopyable &operator=(const Scsi_Command_Is_Noncopyable &);
+public:
+    Scsi_Command_Is_Noncopyable() {}
+};
 	
 #if defined(__linux)
 
@@ -205,7 +213,7 @@
 { return rsm_open_device.f!=open; }
 #endif
 
-class Scsi_Command {
+class Scsi_Command : public Scsi_Command_Is_Noncopyable {
 private:
     int fd,autoclose;
     char *filename;
@@ -389,7 +397,7 @@
 		WRITE=SCCMD_WRITE
 	     } Direction;
 
-class Scsi_Command {
+class Scsi_Command : public Scsi_Command_Is_Noncopyable {
 private:
     int fd,autoclose;
     char *filename;
@@ -510,7 +518,7 @@
 		WRITE=CAM_DIR_OUT
 	     } Direction;
 
-class Scsi_Command {
+class Scsi_Command : public Scsi_Command_Is_Noncopyable {
 private:
     int fd,autoclose;
     char *filename;
@@ -701,7 +709,7 @@
 		WRITE=USCSI_WRITE
 	     } Direction;
 
-class Scsi_Command {
+class Scsi_Command : public Scsi_Command_Is_Noncopyable {
 private:
     int fd,autoclose;
     char *filename;
@@ -920,7 +928,7 @@
                 WRITE=0
              } Direction;
 
-class Scsi_Command {
+class Scsi_Command : public Scsi_Command_Is_Noncopyable {
 private:
     int fd;
     int autoclose;
@@ -1077,7 +1085,7 @@
 		WRITE=DSRQ_WRITE
 	     } Direction;
 
-class Scsi_Command {
+class Scsi_Command : public Scsi_Command_Is_Noncopyable {
 private:
     int fd,autoclose;
     char *filename;
@@ -1277,7 +1285,7 @@
     unsigned char		sense[18];
 } SPKG;
 
-class Scsi_Command {
+class Scsi_Command : public Scsi_Command_Is_Noncopyable {
 private:
     HANDLE fd;
     int    autoclose;
@@ -1449,7 +1457,7 @@
 		WRITE=kSCSIDataTransfer_FromInitiatorToTarget
 	     } Direction;
 
-class Scsi_Command {
+class Scsi_Command : public Scsi_Command_Is_Noncopyable {
 private:
     int				autoclose,_timeout;
     char			*filename;