[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [PATCH 3/4] raw-posix: Introduce hdev_is_sg()
From: |
Paolo Bonzini |
Subject: |
Re: [Qemu-block] [PATCH 3/4] raw-posix: Introduce hdev_is_sg() |
Date: |
Fri, 08 May 2015 13:42:59 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.5.0 |
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 <address@hidden>
> ---
> 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) {
>