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 09:18:59AM +0000, Ratheesh Kannoth wrote:
> > > > > > +	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) {
> >    ...
> >    }
> > }
> ACK.   
> instead of errno != EXIST ,  you may consider stat() before mkdir() call. Just my way thinking(please ignore it, if you don't like). 
> My thinking is --> you need to execute mkdir () only first time, second time onwards, stat() call will return 0.

That's racy: we can stat and have a non existing folder, then have
another arpd instance (or anything else, really) create the directory,
and we would hit EEXIST anyway when we call mkdir.
Also, that needs two syscalls instead of one.

-- 
Max Gautier