Patchwork raw-posix: improve detection of scsi-generic devices

login
register
mail settings
Submitter Bernhard Kohl
Date Sept. 6, 2010, 3:06 p.m.
Message ID <1283785562-31830-1-git-send-email-bernhard.kohl@nsn.com>
Download mbox | patch
Permalink /patch/63928/
State New
Headers show

Comments

Bernhard Kohl - Sept. 6, 2010, 3:06 p.m.
From: Bernhard Kohl <bernhard.kohl@gmx.net>

Allow symbolic links which point to /dev/sgX devices.

Signed-off-by: Bernhard Kohl <bernhard.kohl@nsn.com>
---
 block/raw-posix.c |   10 ++++++++--
 1 files changed, 8 insertions(+), 2 deletions(-)
Alexander Graf - Sept. 6, 2010, 3:39 p.m.
On 06.09.2010, at 17:06, Bernhard Kohl wrote:

> From: Bernhard Kohl <bernhard.kohl@gmx.net>
> 
> Allow symbolic links which point to /dev/sgX devices.

Couldn't you send an SG_IO test ioctl over and see if it works? I really dislike the whole file name magic matching.


Alex
Christoph Hellwig - Sept. 7, 2010, 2:04 a.m.
On Mon, Sep 06, 2010 at 05:39:00PM +0200, Alexander Graf wrote:
> 
> On 06.09.2010, at 17:06, Bernhard Kohl wrote:
> 
> > From: Bernhard Kohl <bernhard.kohl@gmx.net>
> > 
> > Allow symbolic links which point to /dev/sgX devices.
> 
> Couldn't you send an SG_IO test ioctl over and see if it works? I really dislike the whole file name magic matching.

You could, but the result would not be what you expect, given that every
/dev/sd* device and more in Linux support it.

What we really need is to stop shoe-horning scsi pass through support
into the block layer.  Once we finally get our generic thread offload
facilily there is no need for it anymore at all, scsi-generic can
simplify offload the SG_IO ioctl and be done with it.  I even have an
old prototype for this, just waiting for the generic thread offload
to get merged before resurrecting it.
Bernhard Kohl - Sept. 7, 2010, 9:40 a.m.
Am 07.09.2010 04:04, schrieb ext Christoph Hellwig:
> On Mon, Sep 06, 2010 at 05:39:00PM +0200, Alexander Graf wrote:
>    
>> On 06.09.2010, at 17:06, Bernhard Kohl wrote:
>>
>>      
>>> From: Bernhard Kohl<bernhard.kohl@gmx.net>
>>>
>>> Allow symbolic links which point to /dev/sgX devices.
>>>        
>> Couldn't you send an SG_IO test ioctl over and see if it works? I really dislike the whole file name magic matching.
>>      
> You could, but the result would not be what you expect, given that every
> /dev/sd* device and more in Linux support it.
>
> What we really need is to stop shoe-horning scsi pass through support
> into the block layer.  Once we finally get our generic thread offload
> facilily there is no need for it anymore at all, scsi-generic can
> simplify offload the SG_IO ioctl and be done with it.  I even have an
> old prototype for this, just waiting for the generic thread offload
> to get merged before resurrecting it.
>    

Wouldn't it be OK to apply this patch in the meantime before we get
this complete new solution? I need to handle about 15 /dev/sg* disks
which are randomly numbered. With this patch I can use the udev
generated links in /dev/disk/by*, which makes life much easier.
Alexander Graf - Sept. 7, 2010, 9:44 a.m.
On 07.09.2010, at 11:40, Bernhard Kohl wrote:

> Am 07.09.2010 04:04, schrieb ext Christoph Hellwig:
>> On Mon, Sep 06, 2010 at 05:39:00PM +0200, Alexander Graf wrote:
>>   
>>> On 06.09.2010, at 17:06, Bernhard Kohl wrote:
>>> 
>>>     
>>>> From: Bernhard Kohl<bernhard.kohl@gmx.net>
>>>> 
>>>> Allow symbolic links which point to /dev/sgX devices.
>>>>       
>>> Couldn't you send an SG_IO test ioctl over and see if it works? I really dislike the whole file name magic matching.
>>>     
>> You could, but the result would not be what you expect, given that every
>> /dev/sd* device and more in Linux support it.
>> 
>> What we really need is to stop shoe-horning scsi pass through support
>> into the block layer.  Once we finally get our generic thread offload
>> facilily there is no need for it anymore at all, scsi-generic can
>> simplify offload the SG_IO ioctl and be done with it.  I even have an
>> old prototype for this, just waiting for the generic thread offload
>> to get merged before resurrecting it.
>>   
> 
> Wouldn't it be OK to apply this patch in the meantime before we get
> this complete new solution? I need to handle about 15 /dev/sg* disks
> which are randomly numbered. With this patch I can use the udev
> generated links in /dev/disk/by*, which makes life much easier.
> 

I agree. I'd take this one in for now, as it improves the situation and then when all world blossoms we can take Christoph's game-changing framework :).

Kevin, what do you think?


Alex
Kevin Wolf - Sept. 7, 2010, 9:47 a.m.
Am 07.09.2010 04:04, schrieb Christoph Hellwig:
> On Mon, Sep 06, 2010 at 05:39:00PM +0200, Alexander Graf wrote:
>>
>> On 06.09.2010, at 17:06, Bernhard Kohl wrote:
>>
>>> From: Bernhard Kohl <bernhard.kohl@gmx.net>
>>>
>>> Allow symbolic links which point to /dev/sgX devices.
>>
>> Couldn't you send an SG_IO test ioctl over and see if it works? I really dislike the whole file name magic matching.
> 
> You could, but the result would not be what you expect, given that every
> /dev/sd* device and more in Linux support it.
> 
> What we really need is to stop shoe-horning scsi pass through support
> into the block layer.  Once we finally get our generic thread offload
> facilily there is no need for it anymore at all, scsi-generic can
> simplify offload the SG_IO ioctl and be done with it.  I even have an
> old prototype for this, just waiting for the generic thread offload
> to get merged before resurrecting it.

Let's apply this patch for the time being then, and remove the code
altogether as soon as possible.

Kevin

Patch

diff --git a/block/raw-posix.c b/block/raw-posix.c
index 72fb8ce..4544a57 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -48,6 +48,7 @@ 
 #endif
 #ifdef __linux__
 #include <sys/ioctl.h>
+#include <sys/param.h>
 #include <linux/cdrom.h>
 #include <linux/fd.h>
 #endif
@@ -868,8 +869,13 @@  static int hdev_open(BlockDriverState *bs, const char *filename, int flags)
 
     s->type = FTYPE_FILE;
 #if defined(__linux__)
-    if (strstart(filename, "/dev/sg", NULL)) {
-        bs->sg = 1;
+    {
+        char resolved_path[ MAXPATHLEN ], *temp;
+
+        temp = realpath(filename, resolved_path);
+        if (temp && strstart(temp, "/dev/sg", NULL)) {
+            bs->sg = 1;
+        }
     }
 #endif