From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932599AbdJ3RUh (ORCPT ); Mon, 30 Oct 2017 13:20:37 -0400 Received: from mx2.suse.de ([195.135.220.15]:48215 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932252AbdJ3RUg (ORCPT ); Mon, 30 Oct 2017 13:20:36 -0400 Date: Mon, 30 Oct 2017 18:20:34 +0100 From: Jan Kara To: Miklos Szeredi Cc: linux-fsdevel@vger.kernel.org, Jan Kara , Amir Goldstein , Xiong Zhou , linux-kernel@vger.kernel.org Subject: Re: [PATCH v2 7/7] fanotify: clean up CONFIG_FANOTIFY_ACCESS_PERMISSIONS ifdefs Message-ID: <20171030172034.GN23278@quack2.suse.cz> References: <1508920899-8115-1-git-send-email-mszeredi@redhat.com> <1508920899-8115-8-git-send-email-mszeredi@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1508920899-8115-8-git-send-email-mszeredi@redhat.com> User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed 25-10-17 10:41:39, Miklos Szeredi wrote: > The only negative from this patch should be an addition of 32bytes to > 'struct fsnotify_group' if CONFIG_FANOTIFY_ACCESS_PERMISSIONS is not > defined. > > Signed-off-by: Miklos Szeredi I like this but some comments below. > @@ -338,11 +332,14 @@ static ssize_t fanotify_read(struct file *file, char __user *buf, > > static ssize_t fanotify_write(struct file *file, const char __user *buf, size_t count, loff_t *pos) > { > -#ifdef CONFIG_FANOTIFY_ACCESS_PERMISSIONS > struct fanotify_response response = { .fd = -1, .response = -1 }; > struct fsnotify_group *group; > int ret; > > + if (!IS_ENABLED(CONFIG_FANOTIFY_ACCESS_PERMISSIONS)) > + return -EINVAL; > + > + Two empty lines here look superfluous. > @@ -358,59 +355,57 @@ static ssize_t fanotify_write(struct file *file, const char __user *buf, size_t > count = ret; > > return count; > -#else > - return -EINVAL; > -#endif > } > > static int fanotify_release(struct inode *ignored, struct file *file) > { > struct fsnotify_group *group = file->private_data; > > -#ifdef CONFIG_FANOTIFY_ACCESS_PERMISSIONS > - struct fanotify_perm_event_info *event, *next; > - struct fsnotify_event *fsn_event; > + if (IS_ENABLED(CONFIG_FANOTIFY_ACCESS_PERMISSIONS)) { > + struct fanotify_perm_event_info *event, *next; > + struct fsnotify_event *fsn_event; Rather than doing this, I'd just let fanotify_release() go through the same path for both CONFIG_FANOTIFY_ACCESS_PERMISSIONS enabled and disabled. Enabled path won't be much more expensive since access_list will be empty and we have to walk & destroy events anyway. That way you also don't have to reindent everything. > @@ -768,10 +763,10 @@ SYSCALL_DEFINE2(fanotify_init, unsigned int, flags, unsigned int, event_f_flags) > if (force_o_largefile()) > event_f_flags |= O_LARGEFILE; > group->fanotify_data.f_flags = event_f_flags; > -#ifdef CONFIG_FANOTIFY_ACCESS_PERMISSIONS > - init_waitqueue_head(&group->fanotify_data.access_waitq); > - INIT_LIST_HEAD(&group->fanotify_data.access_list); > -#endif > + if (IS_ENABLED(CONFIG_FANOTIFY_ACCESS_PERMISSIONS)) { > + init_waitqueue_head(&group->fanotify_data.access_waitq); > + INIT_LIST_HEAD(&group->fanotify_data.access_list); > + } When having space for these allocated, just initialize them properly. Otherwise it's asking for trouble. Honza -- Jan Kara SUSE Labs, CR