Re: [PATCH 2.6.15-rc6] block: Make CDROMEJECT more robust



On Mon, Dec 19 2005, Ben Collins wrote:
> On Mon, 2005-12-19 at 20:35 +0100, Jens Axboe wrote:
> > On Mon, Dec 19 2005, Ben Collins wrote:
> > > Reference: https://bugzilla.ubuntu.com/5049
> > >
> > > The eject command was failing for a large group of users for removable
> > > devices. The "eject -r" command, which uses the CDROMEJECT ioctl would not
> > > work, however "eject -s", which uses SG_IO did work, but required root access.
> > >
> > > Since SG_IO was using the same mechanism as CDROMEJECT, there should be no
> > > difference. The main reason for getting the CDROMEJECT ioctl working was
> > > because it didn't need root privileges like the SG_IO commands did.
> > >
> > > One bug was noticed, and that is CDROMEJECT was setting the blk request to a
> > > WRITE operation, when in fact it wasn't. The block layer did not like getting
> > > WRITE requests when data_len==0 and data==NULL.
> >
> > False, it can't be a write request if there's no data attached. Write is
> > simply used there because read requests are usually more precious.
>
> Did you mean "can be a write request"? If not, then you just repeated
> what I said.

No, you are misreading me. Definition:

- Read request: direction bit not set, non-zero data length
- Write request: direction bit set, non-zero data length

How can you read or write anything, when there's nothing to read or
write?

> > > This patch fixes the WRITE vs READ issue, and also sends the extra two
> > > commands. Anyone with an iPod connected via USB (not sure about firewire)
> > > should be able to reproduce this issue, and verify the patch.
> >
> > The bug was in the SCSI layer, and James already has the fix integrated
> > for that. It really should make 2.6.15, James are you sending it upwards
> > for that?
>
> Can you point me to this fix? Also, does the "fix" fix the case for IDE
> CDROM's too?

What is the problem case for for ide-cd?

The SCSI fix is here:

http://kernel.org/git/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=a8c730e85e80734412f4f73ab28496a0e8b04a7b

and followed up by a fix for James to cater to all paths:

http://kernel.org/git/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=c9526497cf03ee775c3a6f8ba62335735f98de7a

> > > case CDROMEJECT:
> > > - rq = blk_get_request(q, WRITE, __GFP_WAIT);
> > > - rq->flags |= REQ_BLOCK_PC;
> > > - rq->data = NULL;
> > > - rq->data_len = 0;
> > > - rq->timeout = BLK_DEFAULT_TIMEOUT;
> > > - memset(rq->cmd, 0, sizeof(rq->cmd));
> > > - rq->cmd[0] = GPCMD_START_STOP_UNIT;
> > > - rq->cmd[4] = 0x02 + (close != 0);
> > > - rq->cmd_len = 6;
> > > - err = blk_execute_rq(q, bd_disk, rq, 0);
> > > - blk_put_request(rq);
> > > + err = 0;
> > > +
> > > + err |= blk_send_allow_medium_removal(q, bd_disk);
> > > + err |= blk_send_start_stop(q, bd_disk, 0x01);
> > > + err |= blk_send_start_stop(q, bd_disk, 0x02);
> >
> > Do this in the eject tool, if it's required for some devices.
>
> It already is in eject tool, but as described, that requires root
> access. Not something I want to force a user to do in order to eject
> their CDROM/iPod/USBStick in gnome. What exactly is wrong with the
> commands? If they are harmless for devices that don't need it, and fix a
> huge number of problems (did you see the Cc list on the bug report?) for
> users with affected devices, then what's the harm?

So the medium removal command does require write permission on the
deviec, but it doesn't require root. If they need to rw to the device
fs, surely they need write permissions on the device in the first place?

--
Jens Axboe

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/



Relevant Pages

  • Re: [PATCH 2.6.15-rc6] block: Make CDROMEJECT more robust
    ... it can't be a write request if there's no data attached. ... my patch does not fix the case for the bug if I leave it as WRITE. ... >>> Do this in the eject tool, if it's required for some devices. ...
    (Linux-Kernel)
  • Re: CD doesnt eject from the drive.
    ... CD doesn't eject from the drive. ... Both "eject" command and "cdcontrol ... completing request directly ... WARNING - READ_TOC freeing taskqueue zombie request ...
    (freebsd-questions)
  • [GIT PULL] block changes for 2.6.28
    ... This is the bulk of the block layer changes for 2.6.28. ... Fix up comments about matching flags between bio and rq ... Add 'discard' request handling ... Support 'discard sectors' operation in translation layer support core ...
    (Linux-Kernel)
  • [GIT PULL] first block round for 2.6.31
    ... cciss: use schedule_timeout_interruptible ... mg_disk: fix dependency on libata ... Add blk_make_request(), takes bio, returns a request ... swim3: dequeue in-flight request ...
    (Linux-Kernel)
  • Re: Notices
    ... An unchecked checkbox is not sent in $_REQUEST and if you do something with $_REQUESTyou'll get a notice. ... I'm a recovering perl programmer so conciseness is still important. ... It's a bit tedious at times to fix harmless missing index notices when you just want to test something. ...
    (comp.lang.php)