[LEDE-DEV] base-files: uci-defaults: do config flush in one shot

Moving a file between tmpfs and other fs is neither
faster nor safer, thus no point in doing it in two steps.

Signed-off-by: Roman Yeryomin <roman@advem.lv>
---
 package/base-files/files/lib/functions/uci-defaults.sh | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

On 02/12/17 00:26, Roman Yeryomin wrote:
> Moving a file between tmpfs and other fs is neither
> faster nor safer, thus no point in doing it in two steps.
>
> Signed-off-by: Roman Yeryomin <roman@advem.lv>

Hi Roman,

The code needs to stay like this. the moment you start the command with 
redirection, the shell will truncate the target file and wont write it 
until a flush() or close() is called. this would leave a window where 
the file is 0 bytes. doing this in 2 steps mitigates that problem.

     John

> ---
>   package/base-files/files/lib/functions/uci-defaults.sh | 3 +--
>   1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/package/base-files/files/lib/functions/uci-defaults.sh b/package/base-files/files/lib/functions/uci-defaults.sh
> index 0cd519a147..8eab5ac1e1 100755
> --- a/package/base-files/files/lib/functions/uci-defaults.sh
> +++ b/package/base-files/files/lib/functions/uci-defaults.sh
> @@ -633,6 +633,5 @@ board_config_update() {
>   }
>   
>   board_config_flush() {
> -	json_dump -i > /tmp/.board.json
> -	mv /tmp/.board.json ${CFG}
> +	json_dump -i > ${CFG}
>   }

On 2017-12-12 11:19, John Crispin wrote:
> 
> 
> On 02/12/17 00:26, Roman Yeryomin wrote:
>> Moving a file between tmpfs and other fs is neither
>> faster nor safer, thus no point in doing it in two steps.
>>
>> Signed-off-by: Roman Yeryomin <roman@advem.lv>
> 
> Hi Roman,
> 
> The code needs to stay like this. the moment you start the command with 
> redirection, the shell will truncate the target file and wont write it 
> until a flush() or close() is called. this would leave a window where 
> the file is 0 bytes. doing this in 2 steps mitigates that problem.
I think Roman has a point that even now it's unsafe and leaves a time
window where the file is incomplete. That said, it's not a good reason
to make this window even bigger. We could easily fix this entirely by
storing the intermediate file in /etc instead of /tmp to ensure that
it's on the same filesystem as the target file.

- Felix

On 12/12/17 12:37, Felix Fietkau wrote:
> On 2017-12-12 11:19, John Crispin wrote:
>>
>> On 02/12/17 00:26, Roman Yeryomin wrote:
>>> Moving a file between tmpfs and other fs is neither
>>> faster nor safer, thus no point in doing it in two steps.
>>>
>>> Signed-off-by: Roman Yeryomin <roman@advem.lv>
>> Hi Roman,
>>
>> The code needs to stay like this. the moment you start the command with
>> redirection, the shell will truncate the target file and wont write it
>> until a flush() or close() is called. this would leave a window where
>> the file is 0 bytes. doing this in 2 steps mitigates that problem.
> I think Roman has a point that even now it's unsafe and leaves a time
> window where the file is incomplete. That said, it's not a good reason
> to make this window even bigger. We could easily fix this entirely by
> storing the intermediate file in /etc instead of /tmp to ensure that
> it's on the same filesystem as the target file.
>
> - Felix

Hi,

or add a -w <file> parameter to json_dump to have it write the file directly

     John

> _______________________________________________
> Lede-dev mailing list
> Lede-dev@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/lede-dev

On 2017-12-12 13:41, John Crispin wrote:
> On 12/12/17 12:37, Felix Fietkau wrote:
>> On 2017-12-12 11:19, John Crispin wrote:
>>> 
>>> On 02/12/17 00:26, Roman Yeryomin wrote:
>>>> Moving a file between tmpfs and other fs is neither
>>>> faster nor safer, thus no point in doing it in two steps.
>>>> 
>>>> Signed-off-by: Roman Yeryomin <roman@advem.lv>
>>> Hi Roman,
>>> 
>>> The code needs to stay like this. the moment you start the command 
>>> with
>>> redirection, the shell will truncate the target file and wont write 
>>> it
>>> until a flush() or close() is called. this would leave a window where
>>> the file is 0 bytes. doing this in 2 steps mitigates that problem.
>> I think Roman has a point that even now it's unsafe and leaves a time
>> window where the file is incomplete. That said, it's not a good reason
>> to make this window even bigger.

Exactly

>> We could easily fix this entirely by
>> storing the intermediate file in /etc instead of /tmp to ensure that
>> it's on the same filesystem as the target file.

Would it make any difference if json_dump already failed and wrote 
incomplete/wrong/none information?

> 
> or add a -w <file> parameter to json_dump to have it write the file 
> directly
> 

Probably this could be faster. But IMO there is no perfect solution. We 
can only introduce error checking using siphash or similar. Though in 
this case it's probably an overkill.
Redirecting output or writing directly from jshn should have similar 
effect.


Regards,
Roman