From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:37774) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YqggT-0004Qq-KB for qemu-devel@nongnu.org; Fri, 08 May 2015 07:43:14 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YqggS-0006oa-Jr for qemu-devel@nongnu.org; Fri, 08 May 2015 07:43:13 -0400 Message-ID: <554CA143.9020405@redhat.com> Date: Fri, 08 May 2015 13:42:59 +0200 From: Paolo Bonzini MIME-Version: 1.0 References: <1431085109-19677-1-git-send-email-dimara@arrikto.com> <1431085109-19677-4-git-send-email-dimara@arrikto.com> In-Reply-To: <1431085109-19677-4-git-send-email-dimara@arrikto.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 3/4] raw-posix: Introduce hdev_is_sg() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Dimitris Aragiorgis , qemu-devel@nongnu.org Cc: kwolf@redhat.com, stefanha@redhat.com, qemu-block@nongnu.org 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 > --- > 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 > #include > #include > +#include > #ifdef __s390__ > #include > #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) { >