Re: [PATCH net-next V3 2/3] tools: bpftool: show filenames of pinned objects

[Jakub Kicinski <jakub.kicinski@netronome.com>]

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!