[U-Boot] [PATCH 2/2] tools: env: Implement atomic replace for filesystem

[Stefano Babic]

On 09/03/2018 10:54, Alex Kiernan wrote:
> On Thu, Mar 8, 2018 at 5:04 PM, Stefano Babic <sbabic at denx.de> wrote:
>> Hi alex,
>>
>> On 08/03/2018 12:52, Alex Kiernan wrote:
>>> If the U-Boot environment is stored in a regular file and redundant
>>> operation isn't set, then write to a temporary file and perform an
>>> atomic rename.
>>>
>>
>> Even if it is not explicitely set (IMHO it should), this code can
>> be used as library and linked to own application. That means that
>> concurrency can happens. I mean:
>>
> 
> At this point you're writing the new environment, so we should hold a
> lock to avoid concurrent writes.

This is already done, even if not in the way I like.
tools/env/fw_env_main.c calls flock() to acquire the resource. This
works using fw_printenv / fw_setenv, but not as library. Library's users
as me have to provide themselves a lock before calling the fw_* functions.

> 
>>> Signed-off-by: Alex Kiernan <alex.kiernan at gmail.com>
>>> ---
>>>
>>>  tools/env/fw_env.c | 81 ++++++++++++++++++++++++++++++++++++++++++++++++++++--
>>>  1 file changed, 78 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/tools/env/fw_env.c b/tools/env/fw_env.c
>>> index 2df3504..b814c4e 100644
>>> --- a/tools/env/fw_env.c
>>> +++ b/tools/env/fw_env.c
>>> @@ -14,6 +14,7 @@
>>>  #include <errno.h>
>>>  #include <env_flags.h>
>>>  #include <fcntl.h>
>>> +#include <libgen.h>
>>>  #include <linux/fs.h>
>>>  #include <linux/stringify.h>
>>>  #include <ctype.h>
>>> @@ -1229,9 +1230,46 @@ static int flash_read (int fd)
>>>       return 0;
>>>  }
>>>
>>> +static int flash_open_tempfile(const char **dname, const char **target_temp)
>>> +{
>>> +     char *dup_name = strdup(DEVNAME (dev_current));
>>> +     char *temp_name = NULL;
>>> +     int rc = -1;
>>> +
>>> +     if (!dup_name)
>>> +             return -1;
>>> +
>>> +     *dname = dirname(dup_name);
>>> +     if (!*dname)
>>> +             goto err;
>>> +
>>> +     rc = asprintf(&temp_name, "%s/uboot.tmp", *dname);
>>
>> Filename is fixed - should we not use mkstemp ?
>>
> 
> I went round all the temp functions before in the end deciding to fix
> it. However it looks like I totally misread the contract that mkstemp
> delivers - I'd thought it didn't pass you the name back, but it
> clearly does; I'll go update it.
> 


Best regards,
Stefano Babic

-- 
=====================================================================
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic at denx.de
=====================================================================