On 08/05/2015 13:38, 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 > > Signed-off-by: Dimitris Aragiorgis <dim...@arrikto.com> > --- > block/raw-posix.c | 39 ++++++++++++++++++++++++++++----------- > 1 file changed, 28 insertions(+), 11 deletions(-) > > diff --git a/block/raw-posix.c b/block/raw-posix.c > index 24b061f..d35e5ac 100644 > --- a/block/raw-posix.c > +++ b/block/raw-posix.c > @@ -57,6 +57,7 @@ > #include <linux/fd.h> > #include <linux/fs.h> > #include <linux/hdreg.h> > +#include <scsi/sg.h> > #ifdef __s390__ > #include <asm/dasd.h> > #endif > @@ -2073,15 +2074,38 @@ static void hdev_parse_filename(const char *filename, > QDict *options, > qdict_put_obj(options, "filename", QOBJECT(qstring_from_str(filename))); > } > > +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)) { > + DEBUG_BLOCK_PRINT("SG device found: type=%d, version=%d\n", > + scsiid.scsi_type, sg_version);
Please replace the DEBUG_BLOCK_PRINT with a tracepoint (see the trace-events file). With this change, the patch is okay and definitely an improvement. Paolo > + return 1; > + } > + > +#endif > + > + return 0; > +} > + > static int hdev_open(BlockDriverState *bs, QDict *options, int flags, > Error **errp) > { > BDRVRawState *s = bs->opaque; > Error *local_err = NULL; > int ret; > - const char *filename = qdict_get_str(options, "filename"); > > #if defined(__APPLE__) && defined(__MACH__) > + const char *filename = qdict_get_str(options, "filename"); > + > if (strstart(filename, "/dev/cdrom", NULL)) { > kern_return_t kernResult; > io_iterator_t mediaIterator; > @@ -2110,16 +2134,6 @@ static int hdev_open(BlockDriverState *bs, QDict > *options, int flags, > #endif > > s->type = FTYPE_FILE; > -#if defined(__linux__) > - { > - char resolved_path[ MAXPATHLEN ], *temp; > - > - temp = realpath(filename, resolved_path); > - if (temp && strstart(temp, "/dev/sg", NULL)) { > - bs->sg = 1; > - } > - } > -#endif > > ret = raw_open_common(bs, options, flags, 0, &local_err); > if (ret < 0) { > @@ -2129,6 +2143,9 @@ static int hdev_open(BlockDriverState *bs, QDict > *options, int flags, > return ret; > } > > + /* Since this does ioctl the device must be already opened */ > + bs->sg = hdev_is_sg(bs); > + > if (flags & BDRV_O_RDWR) { > ret = check_hdev_writable(s); > if (ret < 0) { >