diff mbox series

[v1,17/22] util/char_dev: Add open_cdev()

Message ID 20230830103754.36461-18-zhenzhong.duan@intel.com (mailing list archive)
State New, archived
Headers show
Series vfio: Adopt iommufd | expand

Commit Message

Duan, Zhenzhong Aug. 30, 2023, 10:37 a.m. UTC 
From: Yi Liu <yi.l.liu@intel.com>

/dev/vfio/devices/vfioX may not exist. In that case it is still possible
to open /dev/char/$major:$minor instead. Add helper function to abstract
the cdev open.

Suggested-by: Jason Gunthorpe <jgg@nvidia.com>
Signed-off-by: Yi Liu <yi.l.liu@intel.com>
Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
---
 MAINTAINERS             |  6 ++++
 include/qemu/char_dev.h | 16 +++++++++++
 util/chardev_open.c     | 61 +++++++++++++++++++++++++++++++++++++++++
 util/meson.build        |  1 +
 4 files changed, 84 insertions(+)
 create mode 100644 include/qemu/char_dev.h
 create mode 100644 util/chardev_open.c

Comments

Daniel P. Berrangé Sept. 20, 2023, 12:39 p.m. UTC | #1 
On Wed, Aug 30, 2023 at 06:37:49PM +0800, Zhenzhong Duan wrote:
> From: Yi Liu <yi.l.liu@intel.com>
> 
> /dev/vfio/devices/vfioX may not exist. In that case it is still possible
> to open /dev/char/$major:$minor instead. Add helper function to abstract
> the cdev open.
> 
> Suggested-by: Jason Gunthorpe <jgg@nvidia.com>
> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
> ---
>  MAINTAINERS             |  6 ++++
>  include/qemu/char_dev.h | 16 +++++++++++
>  util/chardev_open.c     | 61 +++++++++++++++++++++++++++++++++++++++++

Using the same naming scheme for the .c and .h is strongly desired.

>  util/meson.build        |  1 +
>  4 files changed, 84 insertions(+)
>  create mode 100644 include/qemu/char_dev.h
>  create mode 100644 util/chardev_open.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 04663fbb6f..74d18593fe 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -3372,6 +3372,12 @@ S: Maintained
>  F: include/qemu/iova-tree.h
>  F: util/iova-tree.c
>  
> +cdev Open
> +M: Yi Liu <yi.l.liu@intel.com>
> +S: Maintained
> +F: include/qemu/char_dev.h
> +F: util/chardev_open.c
> +


> diff --git a/util/chardev_open.c b/util/chardev_open.c
> new file mode 100644
> index 0000000000..d03e415131
> --- /dev/null
> +++ b/util/chardev_open.c
> @@ -0,0 +1,61 @@
> +/*
> + * Copyright (C) 2023 Intel Corporation.
> + * Copyright (c) 2019, Mellanox Technologies. All rights reserved.
> + *
> + * Authors: Yi Liu <yi.l.liu@intel.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2.  See
> + * the COPYING file in the top-level directory.
> + *
> + * Copied from
> + * https://github.com/linux-rdma/rdma-core/blob/master/util/open_cdev.c
> + *
> + */

Since this is GPL-2.0-only, IMHO it would be preferrable to keep it
out of the util/ directory, as we're aiming to not add further 2.0
only code, except for specific subdirs. This only appears to be used
by code under hw/vfio/, whcih is one of the dirs still permitting
2.0-only code. So I think better to keep this file where it is used.

> +#ifndef _GNU_SOURCE
> +#define _GNU_SOURCE
> +#endif

This is set globally for building all files in QEMU

> +#include "qemu/osdep.h"
> +#include "qemu/char_dev.h"
> +
> +static int open_cdev_internal(const char *path, dev_t cdev)
> +{
> +    struct stat st;
> +    int fd;
> +
> +    fd = qemu_open_old(path, O_RDWR);
> +    if (fd == -1) {
> +        return -1;
> +    }
> +    if (fstat(fd, &st) || !S_ISCHR(st.st_mode) ||
> +        (cdev != 0 && st.st_rdev != cdev)) {
> +        close(fd);
> +        return -1;
> +    }
> +    return fd;
> +}
> +
> +static int open_cdev_robust(dev_t cdev)
> +{
> +    char *devpath;

g_autofree for this...

> +    int ret;
> +
> +    /*
> +     * This assumes that udev is being used and is creating the /dev/char/
> +     * symlinks.
> +     */
> +    devpath = g_strdup_printf("/dev/char/%u:%u", major(cdev), minor(cdev));
> +    ret = open_cdev_internal(devpath, cdev);
> +    g_free(devpath);

...avoids the need for g_free, and also avoids the need for
the intermediate 'ret' variable.

> +    return ret;
> +}
> +
> +int open_cdev(const char *devpath, dev_t cdev)
> +{
> +    int fd;
> +
> +    fd = open_cdev_internal(devpath, cdev);
> +    if (fd == -1 && cdev != 0) {
> +        return open_cdev_robust(cdev);
> +    }
> +    return fd;
> +}
> diff --git a/util/meson.build b/util/meson.build
> index a375160286..d5313d858f 100644
> --- a/util/meson.build
> +++ b/util/meson.build
> @@ -107,6 +107,7 @@ if have_block
>      util_ss.add(files('filemonitor-stub.c'))
>    endif
>    util_ss.add(when: 'CONFIG_LINUX', if_true: files('vfio-helpers.c'))
> +  util_ss.add(when: 'CONFIG_LINUX', if_true: files('chardev_open.c'))
>  endif
>  
>  if cpu == 'aarch64'
> -- 
> 2.34.1
> 
> 

With regards,
Daniel
Jason Gunthorpe Sept. 20, 2023, 12:53 p.m. UTC | #2 
On Wed, Sep 20, 2023 at 01:39:02PM +0100, Daniel P. Berrangé wrote:

> > diff --git a/util/chardev_open.c b/util/chardev_open.c
> > new file mode 100644
> > index 0000000000..d03e415131
> > --- /dev/null
> > +++ b/util/chardev_open.c
> > @@ -0,0 +1,61 @@
> > +/*
> > + * Copyright (C) 2023 Intel Corporation.
> > + * Copyright (c) 2019, Mellanox Technologies. All rights reserved.
> > + *
> > + * Authors: Yi Liu <yi.l.liu@intel.com>
> > + *
> > + * This work is licensed under the terms of the GNU GPL, version 2.  See
> > + * the COPYING file in the top-level directory.
> > + *
> > + * Copied from
> > + * https://github.com/linux-rdma/rdma-core/blob/master/util/open_cdev.c
> > + *
> > + */
> 
> Since this is GPL-2.0-only, IMHO it would be preferrable to keep it
> out of the util/ directory, as we're aiming to not add further 2.0
> only code, except for specific subdirs. This only appears to be used
> by code under hw/vfio/, whcih is one of the dirs still permitting
> 2.0-only code. So I think better to keep this file where it is used.

The copyright comment above is not fully accurate.

The original code is under the "OpenIB" dual license, you can choose
to take it using the OpenIB BSD license text:

 *      Redistribution and use in source and binary forms, with or
 *      without modification, are permitted provided that the following
 *      conditions are met:
 *
 *      - Redistributions of source code must retain the above
 *        copyright notice, this list of conditions and the following
 *        disclaimer.
 *
 *      - Redistributions in binary form must reproduce the above
 *        copyright notice, this list of conditions and the following
 *        disclaimer in the documentation and/or other materials
 *        provided with the distribution.
 *
 * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
 * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
 * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
 * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS
 * BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN
 * ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN
 * CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
 * SOFTWARE.

And drop reference to GPL if that is what qemu desires.

Jason
Daniel P. Berrangé Sept. 20, 2023, 12:56 p.m. UTC | #3 
On Wed, Sep 20, 2023 at 09:53:46AM -0300, Jason Gunthorpe wrote:
> On Wed, Sep 20, 2023 at 01:39:02PM +0100, Daniel P. Berrangé wrote:
> 
> > > diff --git a/util/chardev_open.c b/util/chardev_open.c
> > > new file mode 100644
> > > index 0000000000..d03e415131
> > > --- /dev/null
> > > +++ b/util/chardev_open.c
> > > @@ -0,0 +1,61 @@
> > > +/*
> > > + * Copyright (C) 2023 Intel Corporation.
> > > + * Copyright (c) 2019, Mellanox Technologies. All rights reserved.
> > > + *
> > > + * Authors: Yi Liu <yi.l.liu@intel.com>
> > > + *
> > > + * This work is licensed under the terms of the GNU GPL, version 2.  See
> > > + * the COPYING file in the top-level directory.
> > > + *
> > > + * Copied from
> > > + * https://github.com/linux-rdma/rdma-core/blob/master/util/open_cdev.c
> > > + *
> > > + */
> > 
> > Since this is GPL-2.0-only, IMHO it would be preferrable to keep it
> > out of the util/ directory, as we're aiming to not add further 2.0
> > only code, except for specific subdirs. This only appears to be used
> > by code under hw/vfio/, whcih is one of the dirs still permitting
> > 2.0-only code. So I think better to keep this file where it is used.
> 
> The copyright comment above is not fully accurate.
> 
> The original code is under the "OpenIB" dual license, you can choose
> to take it using the OpenIB BSD license text:
> 
>  *      Redistribution and use in source and binary forms, with or
>  *      without modification, are permitted provided that the following
>  *      conditions are met:
>  *
>  *      - Redistributions of source code must retain the above
>  *        copyright notice, this list of conditions and the following
>  *        disclaimer.
>  *
>  *      - Redistributions in binary form must reproduce the above
>  *        copyright notice, this list of conditions and the following
>  *        disclaimer in the documentation and/or other materials
>  *        provided with the distribution.
>  *
>  * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
>  * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
>  * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
>  * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS
>  * BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN
>  * ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN
>  * CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
>  * SOFTWARE.
> 
> And drop reference to GPL if that is what qemu desires.

Simplest is probably just to copy the original license header as-is,
and thus preserve the GPL OR BSD choice.

With regards,
Daniel
Duan, Zhenzhong Sept. 21, 2023, 2:37 a.m. UTC | #4 
>-----Original Message-----
>From: Daniel P. Berrangé <berrange@redhat.com>
>Sent: Wednesday, September 20, 2023 8:39 PM
>Subject: Re: [PATCH v1 17/22] util/char_dev: Add open_cdev()
>
>On Wed, Aug 30, 2023 at 06:37:49PM +0800, Zhenzhong Duan wrote:
>> From: Yi Liu <yi.l.liu@intel.com>
>>
>> /dev/vfio/devices/vfioX may not exist. In that case it is still possible
>> to open /dev/char/$major:$minor instead. Add helper function to abstract
>> the cdev open.
>>
>> Suggested-by: Jason Gunthorpe <jgg@nvidia.com>
>> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
>> ---
>>  MAINTAINERS             |  6 ++++
>>  include/qemu/char_dev.h | 16 +++++++++++
>>  util/chardev_open.c     | 61 +++++++++++++++++++++++++++++++++++++++++
>
>Using the same naming scheme for the .c and .h is strongly desired.

Got it.

>
>>  util/meson.build        |  1 +
>>  4 files changed, 84 insertions(+)
>>  create mode 100644 include/qemu/char_dev.h
>>  create mode 100644 util/chardev_open.c
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 04663fbb6f..74d18593fe 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -3372,6 +3372,12 @@ S: Maintained
>>  F: include/qemu/iova-tree.h
>>  F: util/iova-tree.c
>>
>> +cdev Open
>> +M: Yi Liu <yi.l.liu@intel.com>
>> +S: Maintained
>> +F: include/qemu/char_dev.h
>> +F: util/chardev_open.c
>> +
>
>
>> diff --git a/util/chardev_open.c b/util/chardev_open.c
>> new file mode 100644
>> index 0000000000..d03e415131
>> --- /dev/null
>> +++ b/util/chardev_open.c
>> @@ -0,0 +1,61 @@
>> +/*
>> + * Copyright (C) 2023 Intel Corporation.
>> + * Copyright (c) 2019, Mellanox Technologies. All rights reserved.
>> + *
>> + * Authors: Yi Liu <yi.l.liu@intel.com>
>> + *
>> + * This work is licensed under the terms of the GNU GPL, version 2.  See
>> + * the COPYING file in the top-level directory.
>> + *
>> + * Copied from
>> + * https://github.com/linux-rdma/rdma-core/blob/master/util/open_cdev.c
>> + *
>> + */
>
>Since this is GPL-2.0-only, IMHO it would be preferrable to keep it
>out of the util/ directory, as we're aiming to not add further 2.0
>only code, except for specific subdirs. This only appears to be used
>by code under hw/vfio/, whcih is one of the dirs still permitting
>2.0-only code. So I think better to keep this file where it is used.

I'll copy the original license header to preserve the GPL OR BSD choice.
As it's not restricted by GPL-2.0-only now, I plan to keep it in util/.
Let me know if you still prefer to move to hw/vifo/.

>
>> +#ifndef _GNU_SOURCE
>> +#define _GNU_SOURCE
>> +#endif
>
>This is set globally for building all files in QEMU

Will remove it.

>
>> +#include "qemu/osdep.h"
>> +#include "qemu/char_dev.h"
>> +
>> +static int open_cdev_internal(const char *path, dev_t cdev)
>> +{
>> +    struct stat st;
>> +    int fd;
>> +
>> +    fd = qemu_open_old(path, O_RDWR);
>> +    if (fd == -1) {
>> +        return -1;
>> +    }
>> +    if (fstat(fd, &st) || !S_ISCHR(st.st_mode) ||
>> +        (cdev != 0 && st.st_rdev != cdev)) {
>> +        close(fd);
>> +        return -1;
>> +    }
>> +    return fd;
>> +}
>> +
>> +static int open_cdev_robust(dev_t cdev)
>> +{
>> +    char *devpath;
>
>g_autofree for this...

Will do.

>
>> +    int ret;
>> +
>> +    /*
>> +     * This assumes that udev is being used and is creating the /dev/char/
>> +     * symlinks.
>> +     */
>> +    devpath = g_strdup_printf("/dev/char/%u:%u", major(cdev), minor(cdev));
>> +    ret = open_cdev_internal(devpath, cdev);
>> +    g_free(devpath);
>
>...avoids the need for g_free, and also avoids the need for
>the intermediate 'ret' variable.

Yes.

Thanks
Zhenzhong
diff mbox series

Patch

diff --git a/MAINTAINERS b/MAINTAINERS
index 04663fbb6f..74d18593fe 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -3372,6 +3372,12 @@  S: Maintained
 F: include/qemu/iova-tree.h
 F: util/iova-tree.c
 
+cdev Open
+M: Yi Liu <yi.l.liu@intel.com>
+S: Maintained
+F: include/qemu/char_dev.h
+F: util/chardev_open.c
+
 elf2dmp
 M: Viktor Prutyanov <viktor.prutyanov@phystech.edu>
 S: Maintained
diff --git a/include/qemu/char_dev.h b/include/qemu/char_dev.h
new file mode 100644
index 0000000000..6580d351c6
--- /dev/null
+++ b/include/qemu/char_dev.h
@@ -0,0 +1,16 @@ 
+/*
+ * QEMU Chardev Helper
+ *
+ * Copyright (C) 2023 Intel Corporation.
+ *
+ * Authors: Yi Liu <yi.l.liu@intel.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2.  See
+ * the COPYING file in the top-level directory.
+ */
+
+#ifndef QEMU_CHARDEV_HELPERS_H
+#define QEMU_CHARDEV_HELPERS_H
+
+int open_cdev(const char *devpath, dev_t cdev);
+#endif
diff --git a/util/chardev_open.c b/util/chardev_open.c
new file mode 100644
index 0000000000..d03e415131
--- /dev/null
+++ b/util/chardev_open.c
@@ -0,0 +1,61 @@ 
+/*
+ * Copyright (C) 2023 Intel Corporation.
+ * Copyright (c) 2019, Mellanox Technologies. All rights reserved.
+ *
+ * Authors: Yi Liu <yi.l.liu@intel.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2.  See
+ * the COPYING file in the top-level directory.
+ *
+ * Copied from
+ * https://github.com/linux-rdma/rdma-core/blob/master/util/open_cdev.c
+ *
+ */
+#ifndef _GNU_SOURCE
+#define _GNU_SOURCE
+#endif
+#include "qemu/osdep.h"
+#include "qemu/char_dev.h"
+
+static int open_cdev_internal(const char *path, dev_t cdev)
+{
+    struct stat st;
+    int fd;
+
+    fd = qemu_open_old(path, O_RDWR);
+    if (fd == -1) {
+        return -1;
+    }
+    if (fstat(fd, &st) || !S_ISCHR(st.st_mode) ||
+        (cdev != 0 && st.st_rdev != cdev)) {
+        close(fd);
+        return -1;
+    }
+    return fd;
+}
+
+static int open_cdev_robust(dev_t cdev)
+{
+    char *devpath;
+    int ret;
+
+    /*
+     * This assumes that udev is being used and is creating the /dev/char/
+     * symlinks.
+     */
+    devpath = g_strdup_printf("/dev/char/%u:%u", major(cdev), minor(cdev));
+    ret = open_cdev_internal(devpath, cdev);
+    g_free(devpath);
+    return ret;
+}
+
+int open_cdev(const char *devpath, dev_t cdev)
+{
+    int fd;
+
+    fd = open_cdev_internal(devpath, cdev);
+    if (fd == -1 && cdev != 0) {
+        return open_cdev_robust(cdev);
+    }
+    return fd;
+}
diff --git a/util/meson.build b/util/meson.build
index a375160286..d5313d858f 100644
--- a/util/meson.build
+++ b/util/meson.build
@@ -107,6 +107,7 @@  if have_block
     util_ss.add(files('filemonitor-stub.c'))
   endif
   util_ss.add(when: 'CONFIG_LINUX', if_true: files('vfio-helpers.c'))
+  util_ss.add(when: 'CONFIG_LINUX', if_true: files('chardev_open.c'))
 endif
 
 if cpu == 'aarch64'