Re: [PATCH] fs: eventfd: fix obsolete comment

Re: [PATCH] fs: eventfd: fix obsolete comment

[linmiaohe]

Matthew Wilcox <willy@infradead.org> wrote:
>On Sat, Dec 07, 2019 at 03:45:33PM +0800, linmiaohe wrote:
>> From: Miaohe Lin <linmiaohe@huawei.com>
>> 
>>   *
>> - * eventfd_fget
>> + * fdget
>
>But this is wrong.  The error pointer is returned from eventfd_ctx_fileget(), not from fdget.
>
>Looking at the three callers of eventfd_ctx_fileget(), I think it would make sense to do this:

Many thanks for your review and nice advice. But I think this patch should belong to you as you found this.
I really did nothing about it. Maybe a Reviewed-by tag for me is enough.

>diff --git a/drivers/vfio/virqfd.c b/drivers/vfio/virqfd.c index 997cb5d0a657..c35b614e3770 100644
>--- a/drivers/vfio/virqfd.c
>+++ b/drivers/vfio/virqfd.c
>@@ -126,11 +126,6 @@ int vfio_virqfd_enable(void *opaque,
> 	INIT_WORK(&virqfd->inject, virqfd_inject);
> 
> 	irqfd = fdget(fd);
>-	if (!irqfd.file) {
>-		ret = -EBADF;
>-		goto err_fd;
>-	}
>-
> 	ctx = eventfd_ctx_fileget(irqfd.file);
> 	if (IS_ERR(ctx)) {
> 		ret = PTR_ERR(ctx);

The err_fd label should further be removed, as after this change, it's no longer used.
Other callers should drop unused jump label too.

>(not even compile tested)

Thanks again.

[PATCH] fs: eventfd: fix obsolete comment

[linmiaohe]

From: Miaohe Lin <linmiaohe@huawei.com>

since commit 36a7411724b1 ("eventfd_ctx_fdget(): use fdget() instead of
fget()"), this comment become outdated and looks confusing. Fix it with
the correct function name.

Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
---
 fs/eventfd.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/eventfd.c b/fs/eventfd.c
index 8aa0ea8c55e8..0b8466b12932 100644
--- a/fs/eventfd.c
+++ b/fs/eventfd.c
@@ -352,7 +352,7 @@ EXPORT_SYMBOL_GPL(eventfd_fget);
  * Returns a pointer to the internal eventfd context, otherwise the error
  * pointers returned by the following functions:
  *
- * eventfd_fget
+ * fdget
  */
 struct eventfd_ctx *eventfd_ctx_fdget(int fd)
 {
-- 
2.19.1

Re: [PATCH] fs: eventfd: fix obsolete comment

[Matthew Wilcox]

On Sat, Dec 07, 2019 at 03:45:33PM +0800, linmiaohe wrote:
> From: Miaohe Lin <linmiaohe@huawei.com>
> 
> since commit 36a7411724b1 ("eventfd_ctx_fdget(): use fdget() instead of
> fget()"), this comment become outdated and looks confusing. Fix it with
> the correct function name.
> 
> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
> ---
>  fs/eventfd.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/eventfd.c b/fs/eventfd.c
> index 8aa0ea8c55e8..0b8466b12932 100644
> --- a/fs/eventfd.c
> +++ b/fs/eventfd.c
> @@ -352,7 +352,7 @@ EXPORT_SYMBOL_GPL(eventfd_fget);
>   * Returns a pointer to the internal eventfd context, otherwise the error
>   * pointers returned by the following functions:
>   *
> - * eventfd_fget
> + * fdget

But this is wrong.  The error pointer is returned from eventfd_ctx_fileget(),
not from fdget.

Looking at the three callers of eventfd_ctx_fileget(), I think it would
make sense to do this:

diff --git a/drivers/vfio/virqfd.c b/drivers/vfio/virqfd.c
index 997cb5d0a657..c35b614e3770 100644
--- a/drivers/vfio/virqfd.c
+++ b/drivers/vfio/virqfd.c
@@ -126,11 +126,6 @@ int vfio_virqfd_enable(void *opaque,
 	INIT_WORK(&virqfd->inject, virqfd_inject);
 
 	irqfd = fdget(fd);
-	if (!irqfd.file) {
-		ret = -EBADF;
-		goto err_fd;
-	}
-
 	ctx = eventfd_ctx_fileget(irqfd.file);
 	if (IS_ERR(ctx)) {
 		ret = PTR_ERR(ctx);
diff --git a/fs/eventfd.c b/fs/eventfd.c
index 8aa0ea8c55e8..d389ffd1dc07 100644
--- a/fs/eventfd.c
+++ b/fs/eventfd.c
@@ -349,17 +349,13 @@ EXPORT_SYMBOL_GPL(eventfd_fget);
  * eventfd_ctx_fdget - Acquires a reference to the internal eventfd context.
  * @fd: [in] Eventfd file descriptor.
  *
- * Returns a pointer to the internal eventfd context, otherwise the error
- * pointers returned by the following functions:
- *
- * eventfd_fget
+ * Returns a pointer to the internal eventfd context, or an error pointer;
+ * see eventfd_ctx_fileget().
  */
 struct eventfd_ctx *eventfd_ctx_fdget(int fd)
 {
 	struct eventfd_ctx *ctx;
 	struct fd f = fdget(fd);
-	if (!f.file)
-		return ERR_PTR(-EBADF);
 	ctx = eventfd_ctx_fileget(f.file);
 	fdput(f);
 	return ctx;
@@ -368,17 +364,18 @@ EXPORT_SYMBOL_GPL(eventfd_ctx_fdget);
 
 /**
  * eventfd_ctx_fileget - Acquires a reference to the internal eventfd context.
- * @file: [in] Eventfd file pointer.
- *
- * Returns a pointer to the internal eventfd context, otherwise the error
- * pointer:
+ * @file: Eventfd file pointer.
  *
- * -EINVAL   : The @fd file descriptor is not an eventfd file.
+ * Return: A pointer to the internal eventfd context, or an error pointer:
+ * * -EBADF  - The @file is NULL.
+ * * -EINVAL - The @file is not an eventfd file.
  */
 struct eventfd_ctx *eventfd_ctx_fileget(struct file *file)
 {
 	struct eventfd_ctx *ctx;
 
+	if (!file)
+		return ERR_PTR(-EBADF);
 	if (file->f_op != &eventfd_fops)
 		return ERR_PTR(-EINVAL);
 
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 01f3f8b665e9..74b45bc439d8 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -4676,11 +4676,6 @@ static ssize_t memcg_write_event_control(struct kernfs_open_file *of,
 	INIT_WORK(&event->remove, memcg_event_remove);
 
 	efile = fdget(efd);
-	if (!efile.file) {
-		ret = -EBADF;
-		goto out_kfree;
-	}
-
 	event->eventfd = eventfd_ctx_fileget(efile.file);
 	if (IS_ERR(event->eventfd)) {
 		ret = PTR_ERR(event->eventfd);
diff --git a/virt/kvm/eventfd.c b/virt/kvm/eventfd.c
index 67b6fc153e9c..814b99c33d44 100644
--- a/virt/kvm/eventfd.c
+++ b/virt/kvm/eventfd.c
@@ -306,11 +306,6 @@ kvm_irqfd_assign(struct kvm *kvm, struct kvm_irqfd *args)
 	seqcount_init(&irqfd->irq_entry_sc);
 
 	f = fdget(args->fd);
-	if (!f.file) {
-		ret = -EBADF;
-		goto out;
-	}
-
 	eventfd = eventfd_ctx_fileget(f.file);
 	if (IS_ERR(eventfd)) {
 		ret = PTR_ERR(eventfd);

(not even compile tested)