From mboxrd@z Thu Jan  1 00:00:00 1970
From: Jakub Kicinski <jakub.kicinski@netronome.com>
Subject: Re: [PATCH net-next V3 2/3] tools: bpftool: show filenames of
 pinned objects
Date: Tue, 7 Nov 2017 20:33:17 -0800
Message-ID: <20171107203317.21b65c01@cakuba>
References: <20171106060632.3380-1-bhole_prashant_q7@lab.ntt.co.jp>
        <20171106060632.3380-3-bhole_prashant_q7@lab.ntt.co.jp>
        <20171107043656.769e4124@cakuba>
        <00d801d3583a$23219ff0$6964dfd0$@lab.ntt.co.jp>
Mime-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Cc: "'David S . Miller'" <davem@davemloft.net>,
        <netdev@vger.kernel.org>, "'Alexei Starovoitov'" <ast@kernel.org>,
        "'Daniel Borkmann'" <daniel@iogearbox.net>,
        "'Quentin Monnet'" <quentin.monnet@netronome.com>
To: "Prashant Bhole" <bhole_prashant_q7@lab.ntt.co.jp>
Return-path: <netdev-owner@vger.kernel.org>
Received: from mail-pg0-f68.google.com ([74.125.83.68]:44423 "EHLO
        mail-pg0-f68.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
        with ESMTP id S1750918AbdKHEdb (ORCPT
        <rfc822;netdev@vger.kernel.org>); Tue, 7 Nov 2017 23:33:31 -0500
Received: by mail-pg0-f68.google.com with SMTP id j3so1066018pga.1
        for <netdev@vger.kernel.org>; Tue, 07 Nov 2017 20:33:31 -0800 (PST)
In-Reply-To: <00d801d3583a$23219ff0$6964dfd0$@lab.ntt.co.jp>
Sender: netdev-owner@vger.kernel.org
List-ID: <netdev.vger.kernel.org>

On Wed, 8 Nov 2017 11:34:44 +0900, Prashant Bhole wrote:
> > > +	FILE *mntfile = NULL;
> > > +	FTSENT *ftse = NULL;
> > > +	FTS *fts = NULL;
> > > +	int fd, err;
> > > +
> > > +	mntfile = setmntent("/proc/mounts", "r");
> > > +	if (!mntfile)
> > > +		return -1;
> > > +
> > > +	while ((mntent = getmntent(mntfile)) != NULL) {  
> > 
> > Please try to avoid comparisons to NULL, writing:
> > 
> > 	if (ptr)
> > 
> > is more intuitive to most C programmers than:
> > 
> > 	if (ptr != NULL)  
> 
> Jakub,
> Thank you for comments. I agree with using 'if (ptr)' for simple check, but
> here it is an assignment.
> It doesn't look intuitive and looks like a typo if I change it to:
>          while ((mntent = getmntent(mntfile))) {

I still prefer this, if you don't mind.

> >   
> > > +		char *path[] = {mntent->mnt_dir, 0};  
> > 
> > Shouldn't there be spaces after and before the curly braces?  Does  
> checkpatch --
> > strict not warn about this?  
> 
> I will add spaces. checkpatch complained about this not being static const,
> but doing so causes compiler warning because it doesn't match with signature
> of fts_open()

Right, that's OK to ignore.  Could you also make the 0 into a NULL,
though?

> I will submit v4 with changes for other comments if you are ok with replies
> above.

Thank you!