Re: [Qemu-block] [Qemu-devel] [PATCH v4 5/5] raw-posix: Introduce hdev_is_sg()

[Dimitris Aragiorgis]

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 <address@hidden> [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?

signature.asc
Description: Digital signature