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

[Denis Kirjanov <dkirjanov@suse.de>]

On 3/18/24 12:26, Max Gautier wrote:
> 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,

Agreed ^^

> and we would hit EEXIST anyway when we call mkdir.
> Also, that needs two syscalls instead of one.
>