Date:   Tue, 7 Mar 2023 18:23:29 +0000
From:   Alok Tiagi <aloktiagi@gmail.com>
To:     David Laight <David.Laight@aculab.com>
Cc:     "viro@zeniv.linux.org.uk" <viro@zeniv.linux.org.uk>,
        "linux-fsdevel@vger.kernel.org" <linux-fsdevel@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "keescook@chromium.org" <keescook@chromium.org>,
        "hch@infradead.org" <hch@infradead.org>,
        Tycho Andersen <tycho@tycho.pizza>
Subject: Re: [RFC 2/3] file: allow callers to free the old file descriptor
 after dup2
Message-ID: <ZAeBIcZvQsB2N7eq@ip-172-31-38-16.us-west-2.compute.internal>
References: <20230302182207.456311-1-aloktiagi@gmail.com>
 <20230302182207.456311-2-aloktiagi@gmail.com>
 <bef11c18fa234948bcab0316418f04aa@AcuMS.aculab.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <bef11c18fa234948bcab0316418f04aa@AcuMS.aculab.com>
Precedence: bulk

On Thu, Mar 02, 2023 at 10:21:11PM +0000, David Laight wrote:
> From: aloktiagi
> > Sent: 02 March 2023 18:22
> > 
> > Allow callers of do_dup2 to free the old file descriptor in case they need to
> > make additional operations on it.
> 
> That doesn't read right at all.
> 
> Whether or not this is a good idea (or can be done differently)
> the interface is horrid.
> 
> >  	if (tofree)
> > -		filp_close(tofree, files);
> > +		*fdfile = tofree;
> 
> Why not:
> 
> 	if (fdfile) [
> 		*fdfile = tofree;
> 	} else {
> 		if (tofree)
> 			filp_close(tofree, files);
> 	}
> 
> Then existing code just passes NULL and the caller can't 'forget'
> to intitalise fdfile.
> 
> 	David
> 
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> Registration No: 1397386 (Wales)
> 

thank you for the review, I'll fix it in v2