Re: [EXTERNAL] Re: [PATCH iproute2-next v2] arpd: create /var/lib/arpd on first use

[Max Gautier <mg@max.gautier.name>]

On Mon, Mar 18, 2024 at 08:51:40AM +0000, Ratheesh Kannoth wrote:
> > From: Max Gautier <mg@max.gautier.name>
> > Sent: Monday, March 18, 2024 2:07 PM
> > To: Ratheesh Kannoth <rkannoth@marvell.com>
> > Cc: netdev@vger.kernel.org
> > Subject: [EXTERNAL] Re: [PATCH iproute2-next v2] arpd: create /var/lib/arpd
> > on first use
> 
> > > > +	if (strcmp(default_dbname, dbname) == 0
> > > > +			&& mkdir(ARPDDIR, 0755) != 0
> > > > +			&& errno != EEXIST
> > > why do you need errno != EEXIST case ? mkdir() will return error in this case
> > as well.
> > 
> > EEXIST is not an error in this case: if the default location already exist, all is
> > good. mkdir would still return -1 in this case, so we need to exclude it
> > manually.
> 
> ACK. IMO, it would make a more readable code if you consider splitting the "if" loop. 

Something like this ? I tend to pack conditions unless branching is
necessary, but no problem if this form is preferred.

if (strcmp(default_dbname, dbname) == 0) {
    if (mkdir(ARPDDIR, 0755) != 0 && errno != EEXIST) {
   ...
   }
}

> 
> 
>   
> > 
> > > > +			) {
> > > > +		perror("create_db_dir");
> > > > +		exit(-1);
> > > > +	}
> > > > +
> > > >  	dbase = dbopen(dbname, O_CREAT|O_RDWR, 0644, DB_HASH,
> > NULL);
> > > >  	if (dbase == NULL) {
> > > >  		perror("db_open");
> > > > --
> > > > 2.44.0
> > > >
> > 
> > --
> > Max Gautier

-- 
Max Gautier