[v4,5/5] raw-posix: Introduce hdev_is_sg()

Until now, an SG device was identified only by checking if its path
started with "/dev/sg". Then, hdev_open() set bs->sg accordingly.
This is very fragile, e.g. it fails with symlinks or relative paths.
We should rely on the actual properties of the device instead of the
specified file path.

Test for an SG device (e.g. /dev/sg0) by ensuring that all of the
following holds:

 - The device supports the SG_GET_VERSION_NUM ioctl
 - The device supports the SG_GET_SCSI_ID ioctl
 - The specified file name corresponds to a character device

Signed-off-by: Dimitris Aragiorgis <dimara@arrikto.com>
---
 block/raw-posix.c |   39 ++++++++++++++++++++++++++++-----------
 1 file changed, 28 insertions(+), 11 deletions(-)

On 05/20/2015 03:57 AM, Dimitris Aragiorgis wrote:
> Until now, an SG device was identified only by checking if its path
> started with "/dev/sg". Then, hdev_open() set bs->sg accordingly.
> This is very fragile, e.g. it fails with symlinks or relative paths.
> We should rely on the actual properties of the device instead of the
> specified file path.
> 
> Test for an SG device (e.g. /dev/sg0) by ensuring that all of the
> following holds:
> 
>  - The device supports the SG_GET_VERSION_NUM ioctl
>  - The device supports the SG_GET_SCSI_ID ioctl
>  - The specified file name corresponds to a character device

I'd check stat first. ioctl's on the wrong device type might have
unexpected side effects.


> +static int hdev_is_sg(BlockDriverState *bs)
> +{
> +
> +#if defined(__linux__)
> +
> +    struct stat st;
> +    struct sg_scsi_id scsiid;
> +    int sg_version;
> +
> +    if (!bdrv_ioctl(bs, SG_GET_VERSION_NUM, &sg_version) &&
> +        !bdrv_ioctl(bs, SG_GET_SCSI_ID, &scsiid) &&
> +        stat(bs->filename, &st) >= 0 && S_ISCHR(st.st_mode)) {
> +        DPRINTF("SG device found: type=%d, version=%d\n",
> +            scsiid.scsi_type, sg_version);
> +        return 1;
> +    }
> +
> +#endif
> +
> +    return 0;

Make this return 'bool', using 'true' and 'false'.  We require a C99
compiler, after all.

On Wed, May 20, 2015 at 12:57:39PM +0300, Dimitris Aragiorgis wrote:
> This is very fragile, e.g. it fails with symlinks or relative paths.

This is not true since realpath(3) is used to resolve symlinks and
product an absolute path.

Is this patch really necessary?

Hello Stefan,

Yes, you are right. Using realpath() is a workaround for supporting
symlinks, as long as they point to a path starting with "/dev/sg". I
will remove this reference in the revised version of this patch.

However, it still holds that determining whether a filename is an SG
device or not is based on textual (and hardcoded) examination, i.e.,
whether it starts with "/dev/sg".

The point is that a device node is a device node no matter what its
filename or its location in the fs tree is. A device node with  major 1,
minor 3 is the null device whether it's called /dev/null or not.

The proposed patch allows the use of any device node which is capable of
acting as a scsi-generic device; it checks for the required intrinsic
property ("does this device support the SG ioctl?") versus trying to
second-guess the nature of the device node by checking its filename.

Similarly, the programming example from SG's documentation does not
check the node of the device node at all:
http://sg.danny.cz/sg/p/sg_v3_ho.html#pexample

    /* It is prudent to check we have a sg device by trying an ioctl */
    if ((ioctl(sg_fd, SG_GET_VERSION_NUM, &k) < 0) || (k < 30000)) {
        printf("%s is not an sg device, or old sg driver\n", argv[1]);
        return 1;

The above has the advantage that it works with device nodes of any name,
and in any directory.

So I suggest we go with the submitted patch taking into account Eric's
proposal: The code first stat()s the given filename to ensure it is a
character device node, and then it issues the SG_GET_VERSION_NUM and
SG_GET_SCSI_ID ioctl()s.

Thanks,
dimara

* Stefan Hajnoczi <stefanha@redhat.com> [2015-06-19 13:33:02 +0100]:

> On Wed, May 20, 2015 at 12:57:39PM +0300, Dimitris Aragiorgis wrote:
> > This is very fragile, e.g. it fails with symlinks or relative paths.
> 
> This is not true since realpath(3) is used to resolve symlinks and
> product an absolute path.
> 
> Is this patch really necessary?

On Mon, Jun 22, 2015 at 01:18:43PM +0300, Dimitris Aragiorgis wrote:
> So I suggest we go with the submitted patch taking into account Eric's
> proposal: The code first stat()s the given filename to ensure it is a
> character device node, and then it issues the SG_GET_VERSION_NUM and
> SG_GET_SCSI_ID ioctl()s.

That's fine.

Regarding Eric's comment about ioctl number collisions, I've checked
Linux Documentation/ioctl/ioctl-number.txt.  At least under Linux there
is no collision for SG_GET_VERSION_NUM/SG_GET_SCSI_ID.

Stefan