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

[Duan, Zhenzhong]

>-----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