[PATCH] raw_handler: Add atomic-install support

[Colin McAllister]

Add property for raw file handler that installs file to .tmp file. Once
the .tmp file is installed, it is atomically renamed to the specified
file in path. This ensures that a partial install does not affect the
currently installed file.

Signed-off-by: Colin McAllister <colin.mc...@garmin.com>
---
doc/source/sw-description.rst | 7 +++++++
handlers/raw_handler.c | 28 +++++++++++++++++++++++++++-
2 files changed, 34 insertions(+), 1 deletion(-)

diff --git a/doc/source/sw-description.rst b/doc/source/sw-description.rst
index 3de3636..6901a72 100644
--- a/doc/source/sw-description.rst
+++ b/doc/source/sw-description.rst
@@ -738,6 +738,13 @@ As a general rule, swupdate doesn't copy out a file if the destination path
doesn't exists. This behavior could be changed using the special property
"create-destination".

+As another general rule, swupdate installs the file directly to the specified
+path. If the destination file is intended to be atomically installed, the
+special property "atomic-install" may be set to "true". This installs the
+file to the specified path with ".tmp" apppended to the filename. In the case
+where a file already exists with the specified path, the atomic install prevents
+the original file from being overwritten until the temp file is installed.
+
Scripts
-------

diff --git a/handlers/raw_handler.c b/handlers/raw_handler.c
index 619a2f4..3fab630 100644
--- a/handlers/raw_handler.c
+++ b/handlers/raw_handler.c
@@ -206,6 +206,7 @@ static int install_raw_file(struct img_type *img,
void __attribute__ ((__unused__)) *data)
{
char path[255];
+ char tmp_path[255];
int fdout;
int ret = 0;
int use_mount = (strlen(img->device) && strlen(img->filesystem)) ? 1 : 0;
@@ -237,6 +238,13 @@ static int install_raw_file(struct img_type *img,
}
}

+ if (strtobool(dict_get_value(&img->properties, "atomic-install"))) {
+ if (snprintf(tmp_path, sizeof(path), "%s%s", img->path, ".tmp") >= (int)sizeof(path)) {
+ ERROR("Temp path too long: %s%s", img->path, ".tmp");
+ return -1;
+ }
+ }
+
TRACE("Installing file %s on %s",
img->fname, path);

@@ -249,7 +257,12 @@ static int install_raw_file(struct img_type *img,
}
}

- fdout = openfileoutput(path);
+ if (strtobool(dict_get_value(&img->properties, "atomic-install"))) {
+ fdout = openfileoutput(tmp_path);
+ }
+ else {
+ fdout = openfileoutput(path);
+ }
if (fdout < 0)
return fdout;
if (!img_check_free_space(img, fdout)) {
@@ -260,8 +273,21 @@ static int install_raw_file(struct img_type *img,
if (ret< 0) {
ERROR("Error copying extracted file");
}
+
+ if (strtobool(dict_get_value(&img->properties, "atomic-install"))) {
+ if(fsync(fdout)) {
+ ERROR("Error writing %s to disk: %s", tmp_path, strerror(errno));
+ }
+ }
+
close(fdout);

+ if (strtobool(dict_get_value(&img->properties, "atomic-install"))) {
+ if(rename(tmp_path, path)) {
+ ERROR("Error renaming %s to %s: %s", tmp_path, path, strerror(errno));
+ }
+ }
+
if (use_mount) {
swupdate_umount(DATADST_DIR);
}
--
2.33.0

[Colin McAllister]

Add property for raw file handler that installs file to .tmp file. Once
the .tmp file is installed, it is atomically renamed to the specified
file in path. This ensures that a partial install does not affect the
currently installed file.

Signed-off-by: Colin McAllister <colin.mc...@garmin.com>
---
doc/source/sw-description.rst | 7 +++++++

handlers/raw_handler.c | 34 +++++++++++++++++++++++++++++++---
2 files changed, 38 insertions(+), 3 deletions(-)

diff --git a/doc/source/sw-description.rst b/doc/source/sw-description.rst
index 3de3636..6901a72 100644
--- a/doc/source/sw-description.rst
+++ b/doc/source/sw-description.rst
@@ -738,6 +738,13 @@ As a general rule, swupdate doesn't copy out a file if the destination path
doesn't exists. This behavior could be changed using the special property
"create-destination".

+As another general rule, swupdate installs the file directly to the specified
+path. If the destination file is intended to be atomically installed, the
+special property "atomic-install" may be set to "true". This installs the
+file to the specified path with ".tmp" apppended to the filename. In the case
+where a file already exists with the specified path, the atomic install prevents
+the original file from being overwritten until the temp file is installed.
+
Scripts
-------

diff --git a/handlers/raw_handler.c b/handlers/raw_handler.c

index 619a2f4..b148189 100644

--- a/handlers/raw_handler.c
+++ b/handlers/raw_handler.c
@@ -206,6 +206,7 @@ static int install_raw_file(struct img_type *img,
void __attribute__ ((__unused__)) *data)
{
char path[255];
+ char tmp_path[255];
int fdout;
int ret = 0;
int use_mount = (strlen(img->device) && strlen(img->filesystem)) ? 1 : 0;

@@ -237,8 +238,16 @@ static int install_raw_file(struct img_type *img,
}
}

- TRACE("Installing file %s on %s",
- img->fname, path);

+ if (strtobool(dict_get_value(&img->properties, "atomic-install"))) {

+ if (snprintf(tmp_path, sizeof(tmp_path), "%s.tmp", img->path) >= (int)sizeof(tmp_path)) {
+ ERROR("Temp path too long: %s.tmp", img->path);
+ return -1;
+ }
+ TRACE("Installing file %s on %s", img->fname, tmp_path);
+ }
+ else {

+ TRACE("Installing file %s on %s", img->fname, path);

+ }

if (strtobool(dict_get_value(&img->properties, "create-destination"))) {
TRACE("Creating path %s", path);
@@ -249,7 +258,12 @@ static int install_raw_file(struct img_type *img,

}
}

- fdout = openfileoutput(path);
+ if (strtobool(dict_get_value(&img->properties, "atomic-install"))) {
+ fdout = openfileoutput(tmp_path);
+ }
+ else {
+ fdout = openfileoutput(path);
+ }
if (fdout < 0)
return fdout;
if (!img_check_free_space(img, fdout)) {

@@ -260,8 +274,22 @@ static int install_raw_file(struct img_type *img,

if (ret< 0) {
ERROR("Error copying extracted file");
}
+
+ if (strtobool(dict_get_value(&img->properties, "atomic-install"))) {
+ if(fsync(fdout)) {
+ ERROR("Error writing %s to disk: %s", tmp_path, strerror(errno));
+ }
+ }
+
close(fdout);

+ if (strtobool(dict_get_value(&img->properties, "atomic-install"))) {

+ TRACE("Renaming file %s to %s", tmp_path, path);

[Stefano Babic]

Hi Colin,

On 03.02.22 22:36, 'Colin McAllister' via swupdate wrote:
> Add property for raw file handler that installs file to .tmp file. Once
> the .tmp file is installed, it is atomically renamed to the specified
> file in path. This ensures that a partial install does not affect the
> currently installed file.
>

Just to understand: it is guaranteed that the whole update is atomic,
but not the single steps. The case above happens when the file you want
to update is unique on the system (maybe a configuration file), and of
course a power cut can happen during the write. But the design is that
the interrupted update is recognized (via flags, see recovery_status)
and system knows that update should repeated.

if you copy here path to tmp_path, you could simplify the code later.

> + }
>
> if (strtobool(dict_get_value(&img->properties, "create-destination"))) {
> TRACE("Creating path %s", path);
> @@ -249,7 +258,12 @@ static int install_raw_file(struct img_type *img,
> }
> }
>
> - fdout = openfileoutput(path);
> + if (strtobool(dict_get_value(&img->properties, "atomic-install"))) {
> + fdout = openfileoutput(tmp_path);
> + }
> + else {
> + fdout = openfileoutput(path);
> + }

You can drop this if you use tmp_path for both cases. It looks more
readable IMO.

> if (fdout < 0)
> return fdout;
> if (!img_check_free_space(img, fdout)) {
> @@ -260,8 +274,22 @@ static int install_raw_file(struct img_type *img,
> if (ret< 0) {
> ERROR("Error copying extracted file");
> }
> +
> + if (strtobool(dict_get_value(&img->properties, "atomic-install"))) {
> + if(fsync(fdout)) {

Is it required ? We have a close(fdout) that implies a sync.

> + ERROR("Error writing %s to disk: %s", tmp_path, strerror(errno));
> + }
> + }
> +
> close(fdout);
>
> + if (strtobool(dict_get_value(&img->properties, "atomic-install"))) {
> + TRACE("Renaming file %s to %s", tmp_path, path);
> + if(rename(tmp_path, path)) {
> + ERROR("Error renaming %s to %s: %s", tmp_path, path, strerror(errno));
> + }
> + }
> +
> if (use_mount) {
> swupdate_umount(DATADST_DIR);
> }

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: sba...@denx.de
=====================================================================

[Christian Storm]

Hi,

> On 03.02.22 22:36, 'Colin McAllister' via swupdate wrote:
> > Add property for raw file handler that installs file to .tmp file. Once
> > the .tmp file is installed, it is atomically renamed to the specified
> > file in path. This ensures that a partial install does not affect the
> > currently installed file.
> >
>
> Just to understand: it is guaranteed that the whole update is atomic, but not
> the single steps. The case above happens when the file you want to update is
> unique on the system (maybe a configuration file), and of course a power cut
> can happen during the write. But the design is that the interrupted update is
> recognized (via flags, see recovery_status) and system knows that update
> should repeated.

How is the <file>.tmp cleaned up in case of an interrupted update?
I understand your old <file> is still in place preserving system
functionality, and so is <file>.tmp, probably with size 0.

Quoting RENAME(2):
"If newpath already exists, it will be atomically replaced, so that
there is no point at which another process attempting to access
newpath will find it missing. However, there will probably be a window
in which both oldpath and newpath refer to the file being renamed."
So, if you're unlucky and get a powercut while this exact point in time,
you end up with both copies. Now, even if unlikely, how is <file>.tmp
cleaned up in this case?

Citing CLOSE(2):
"A successful close does not guarantee that the data has been
successfully saved to disk, as the kernel uses the buffer cache to
defer writes. Typically, filesystems do not flush buffers when a file
is closed. If you need to be sure that the data is physically stored on
the underlying disk, use fsync(2). (It will depend on the disk
hardware at this point.)"

So, yes, you need to flush it down to disk so that rename() is atomic, if
you haven't mounted sync, that is. And it's going down a rabbit hole on
remote filesystems...

> > + ERROR("Error writing %s to disk: %s", tmp_path, strerror(errno));
> > + }
> > + }
> > +
> > close(fdout);
> > + if (strtobool(dict_get_value(&img->properties, "atomic-install"))) {
> > + TRACE("Renaming file %s to %s", tmp_path, path);
> > + if(rename(tmp_path, path)) {
> > + ERROR("Error renaming %s to %s: %s", tmp_path, path, strerror(errno));
> > + }
> > + }
> > +
> > if (use_mount) {
> > swupdate_umount(DATADST_DIR);
> > }

Kind regards,
Christian

--
Dr. Christian Storm
Siemens AG, Technology, T CED SES-DE
Otto-Hahn-Ring 6, 81739 München, Germany

[Colin McAllister]

Add property for raw file handler that installs file to .tmp file. Once
the .tmp file is installed, it is atomically renamed to the specified
file in path. This ensures that a partial install does not affect the
currently installed file.

Change-Id: I4934a6bc71d95a181c5f48769ae299b4d57232ba

Signed-off-by: Colin McAllister <colin.mc...@garmin.com>
---
doc/source/sw-description.rst | 7 +++++++

handlers/raw_handler.c | 29 ++++++++++++++++++++++++++---
2 files changed, 33 insertions(+), 3 deletions(-)

diff --git a/doc/source/sw-description.rst b/doc/source/sw-description.rst
index 3de3636..6901a72 100644
--- a/doc/source/sw-description.rst
+++ b/doc/source/sw-description.rst
@@ -738,6 +738,13 @@ As a general rule, swupdate doesn't copy out a file if the destination path
doesn't exists. This behavior could be changed using the special property
"create-destination".

+As another general rule, swupdate installs the file directly to the specified
+path. If the destination file is intended to be atomically installed, the
+special property "atomic-install" may be set to "true". This installs the
+file to the specified path with ".tmp" apppended to the filename. In the case
+where a file already exists with the specified path, the atomic install prevents
+the original file from being overwritten until the temp file is installed.
+
Scripts
-------

diff --git a/handlers/raw_handler.c b/handlers/raw_handler.c

index 619a2f4..c9ff851 100644

--- a/handlers/raw_handler.c
+++ b/handlers/raw_handler.c
@@ -206,6 +206,7 @@ static int install_raw_file(struct img_type *img,
void __attribute__ ((__unused__)) *data)
{
char path[255];
+ char tmp_path[255];
int fdout;
int ret = 0;
int use_mount = (strlen(img->device) && strlen(img->filesystem)) ? 1 : 0;

@@ -237,8 +238,16 @@ static int install_raw_file(struct img_type *img,
}
}

- TRACE("Installing file %s on %s",
- img->fname, path);

+ if (strtobool(dict_get_value(&img->properties, "atomic-install"))) {

+ if (snprintf(tmp_path, sizeof(tmp_path), "%s.tmp", path) >= (int)sizeof(tmp_path)) {
+ ERROR("Temp path too long: %s.tmp", img->path);

+ return -1;
+ }
+ }

+ else {
+ snprintf(tmp_path, sizeof(tmp_path), "%s", path);
+ }
+ TRACE("Installing file %s on %s", img->fname, tmp_path);
Fixed this as requested.

if (strtobool(dict_get_value(&img->properties, "create-destination"))) {
TRACE("Creating path %s", path);

@@ -249,7 +258,7 @@ static int install_raw_file(struct img_type *img,
}
}

- fdout = openfileoutput(path);
+ fdout = openfileoutput(tmp_path);

if (fdout < 0)
return fdout;
if (!img_check_free_space(img, fdout)) {

@@ -260,8 +269,22 @@ static int install_raw_file(struct img_type *img,

if (ret< 0) {
ERROR("Error copying extracted file");
}
+
+ if (strtobool(dict_get_value(&img->properties, "atomic-install"))) {
+ if(fsync(fdout)) {
+ ERROR("Error writing %s to disk: %s", tmp_path, strerror(errno));
+ }
+ }
+

Based on the spec, close is not guaranteed to write a file to disk and
does not imply a sync, so that's why I added this here. I also know
that this is a time costly operation, hence why I conditionally do this
only if atomic-install is set.

close(fdout);

+ if (strtobool(dict_get_value(&img->properties, "atomic-install"))) {

+ TRACE("Renaming file %s to %s", tmp_path, path);

[Colin McAllister]

On Friday, February 4, 2022 at 9:14:41 AM UTC-6 Christian Storm wrote:

Hi,

> On 03.02.22 22:36, 'Colin McAllister' via swupdate wrote:
> > Add property for raw file handler that installs file to .tmp file. Once
> > the .tmp file is installed, it is atomically renamed to the specified
> > file in path. This ensures that a partial install does not affect the
> > currently installed file.
> >
>
> Just to understand: it is guaranteed that the whole update is atomic, but not
> the single steps. The case above happens when the file you want to update is
> unique on the system (maybe a configuration file), and of course a power cut
> can happen during the write. But the design is that the interrupted update is
> recognized (via flags, see recovery_status) and system knows that update
> should repeated.

The point of this patch is to provide a feature that ensures that designated files

will not and should never be left in a corrupted state by power loss or some other

unexpected failure.

 

How is the <file>.tmp cleaned up in case of an interrupted update?
I understand your old <file> is still in place preserving system
functionality, and so is <file>.tmp, probably with size 0.

With this patch the tmp file is not cleaned up in the case of an interrupted update.

I think the complexity of cleaning up a tmp file would be difficult as this step would

have to be performed on next boot.  We would likely need some sort of database to

keep track of what to clean up, which I think goes beyond the scope of swupdate.

I think an implementer could have their own tools installed on their rootfs to clean

up tmp files if that is an issue.

To Stefano's point, the update would be interrupted, and since the whole update is

atomic, another update would need to be performed to update the system.

Running the update again would overwrite the tmp file before it is renamed over the

target file, essentially removing the tmp file.

[Stefano Babic]

Hi Colin,

But we cannot be sure that at next boot which between new and old
software is started, and there will be a dependency in some way if
production software should make housekeeping of the update process.

>  We would likely need some sort of
> database

No.

> to
> keep track of what to clean up, which I think goes beyond the scope of
> swupdate.

A complete rollback of temporary installation becomes complex and it is
not needed during an update - see rollback of transaction for databases.
But see again my comment: if you face such as problem, it does mean that
you are trying to update in a not atomic way or you are not taking into
account the transaction flags (recovery_status and ustate variables)
provided by SWUpdate. You are trying to fix the case where a single file
(maybe a configuration ?) is updated. But what does it happen if more as
one file must be updated ? And if you have multiple files, it makes
sense to deploy them as tarball and then let the "archive" handler to
install them. An interrupted update (due for example to a power-cut) can
generate a lot of different states (depending on which file was not
installed). None of these really matters: the transaction was not
committed and the old SW should run again, or the device is waiting
again for an update. An in-the-middle state just means you do not bother
with SWUpdate's transaction, and you try to fix the very simple case by
updating a single file.

You are posting this patch reporting as issue that system remains in a
corrupted state: but this is not true, it depends on the update concept
you are implementing. You are just mitigating and reducing the time
window when this can happen.

> I think an implementer could have their own tools installed on their
> rootfs to clean
> up tmp files if that is an issue.

I disagree here - it creates a lot of dependencies between the updater
and the software that runs. For example, a new file is updated but for
some reasons the new SW after a reboot does not run, and there is a
fallback to the (running at the time of the update) version. How can an
older version know about new files coming from a newer version ?

> To Stefano's point, the update would be interrupted, and since the whole
> update is
> atomic, another update would need to be performed to update the system.

I guess my point was misunderstood - it does not matter if the file is
partially updated or not (the issue you report), because the "update
transaction" was not completed successfully, and that means everything
installed during an update is simply garbage and it should not
considered ready-to-run. The issue you report happens if you are
updating "live", that is replacing a file currently used by the running
software, without taking care if the update transaction completed with
success.

>
> Quoting RENAME(2):
> "If newpath already exists, it will be atomically replaced, so that
> there is no point at which another process attempting to access
> newpath will find it missing. However, there will probably be a window
> in which both oldpath and newpath refer to the file being renamed."
> So, if you're unlucky and get a powercut while this exact point in
> time,
> you end up with both copies.

This can still happen, but the time window is reduced.

[Colin McAllister]

Hi Stefano,

Sorry, I didn't reply all again...

> You are just mitigating and reducing the time
> window when this can happen.

Yes, this is correct.  There are some use cases we have identified where a large file needs to be updated. The probability of a partially written or empty file greatly increases as the file size increases. This patch intends to add a feature that significantly minimizes the chance that a larger file is left in a corrupted state.

> I disagree here - it creates a lot of dependencies between the updater
> and the software that runs. For example, a new file is updated but for
> some reasons the new SW after a reboot does not run, and there is a
> fallback to the (running at the time of the update) version. How can an
> older version know about new files coming from a newer version ?

Breaking compatibility is a problem that could occur, but there are many ways to handle that.

There are use cases where an empty or corrupted file is worse than having the older file.  Our specific use case is when we only update update ancillary data and not the whole system.  In this case we are not worried about API compatibility between the file and the rest of the system.  Instead, we are concerned about the state of the system being left in a corrupted state with blank or partially written files.

The atomic flag proposed in this patch allows the writer of the swupdate file to make the decision if a partial or corrupted file is worse than an old copy of the file.

Thanks,

Colin

[Stefano Babic]

Hi Colin,

On 07.02.22 16:41, 'Colin McAllister' via swupdate wrote:
> Hi Stefano,
>
> Sorry, I didn't reply all again...
>
>> You are just mitigating and reducing the time
>> window when this can happen.
>
>
> Yes, this is correct.  There are some use cases we have identified where
> a large file needs to be updated. The probability of a partially written
> or empty file greatly increases as the file size increases.

Of course.

> This patch
> intends to add a feature that significantly minimizes the chance that a
> larger file is left in a corrupted state.

Yes, but there is still a chance that file gets corrupted.

The rename is done at filesystem level - change is propagated to
lowlevel to the storage driver, maybe a SDHC driver that split the
changes from SWUpdate in multiple commands (or multiple blocks), and
they are not atomic, too. So we can say reduce the risk, but it cannot
eliminate it.

>
>
>> I disagree here - it creates a lot of dependencies between the updater
>> and the software that runs. For example, a new file is updated but for
>> some reasons the new SW after a reboot does not run, and there is a
>> fallback to the (running at the time of the update) version. How can an
>> older version know about new files coming from a newer version ?
>
> Breaking compatibility is a problem that could occur, but there are many
> ways to handle that.
>
> There are use cases where an empty or corrupted file is worse than
> having the older file.

Absolutely - it should never happen that systems runs after an
incomplete update. But this is exactly your case.

>  Our specific use case is when we only update
> update ancillary data and not the whole system.

This is my point - your system update can be interrupted, it has surely
much larger files as in the case you report, but it does not happen (I
hope !!) that an uncompleted update can run, if your integration is correct.

On the other side, you update a single large file, and you see you get
into trouble because the updated file is corrupted. You see a
discrepancy here. If this is a generic issue, it could often happen on a
system update because SWU / files are usually much larger. But a system
update is treated by you as transaction, and until SWUpdate does not
commit the transaction, the updated software is just garbage. It does
not matter if files are partially written. The transaction flag is a
commit to signal that all files are correctly installed. This seems to
be missing in your design.

If you update a single file, you should create the atomicity yourself.
For example, still having two copies (current and standby), and let them
toggling in a safe environment (it could be by stopping the app instead
of rebooting), etc. This really depends on the project and I agree that
I am speculating because I do not know the details, but it is just to
tell you that an incompleted update can happen in any conditions
(power-cut, crash, etc.). The state machine and transaction flags in
SWUpdate are set if an update is valid or not. In your case, it works if
the update of the single file is not declared valid until SWUpdate
terminates with success.

>  In this case we are not
> worried about API compatibility between the file and the rest of the
> system.  Instead, we are concerned about the state of the system being
> left in a corrupted state with blank or partially written files.

Again, this is due to your update concept for this single file. It
becomes much worse if you update from a tarball that imply many files. I
guess you are updating a "live" system.

>
> The atomic flag proposed in this patch allows the writer of the swupdate
> file to make the decision if a partial or corrupted file is worse than
> an old copy of the file.

I could merge this even if I think that it is just a work-around for a
not safe update concept. It is still compatible with the past, and a
user can simply decide to not use it.

Anyway, the commit message is not correct: " Once

the .tmp file is installed, it is atomically renamed to the specified
file in path. This ensures that a partial install does not affect the

currently installed file.".

This can confuse and make thinking that any update using the rawfile
handler leads to corrupted data - this is not true, it is just that the
update itself is considered atomic (ok or not ok), but not the run of a
single handler (we can say the same for each handler), so please
reformulate.

Best regards,
Stefano

> Phone: +49-8142-66989-53 <tel:+49%208142%206698953> Fax:
> +49-8142-66989-80 <tel:+49%208142%206698980> Email: sba...@denx.de
> =====================================================================
>
> --
> You received this message because you are subscribed to the Google
> Groups "swupdate" group.
> To unsubscribe from this group and stop receiving emails from it, send
> an email to swupdate+u...@googlegroups.com
> <mailto:swupdate+u...@googlegroups.com>.
> To view this discussion on the web visit
> https://groups.google.com/d/msgid/swupdate/d77596aa-4db5-48a9-9a11-9099fba5a5c5n%40googlegroups.com
> <https://groups.google.com/d/msgid/swupdate/d77596aa-4db5-48a9-9a11-9099fba5a5c5n%40googlegroups.com?utm_medium=email&utm_source=footer>.

[Colin McAllister]

Add property for raw file handler that installs file to .tmp file. Once

the .tmp file is installed and the write buffer is flushed, the file is
renamed to the specified file in path. This minimizes the chance of a
corrupted file if the raw file handler is interrupted.

Signed-off-by: Colin McAllister <colin.mc...@garmin.com>
---

doc/source/sw-description.rst | 11 +++++++++++
handlers/raw_handler.c | 29 ++++++++++++++++++++++++++---
2 files changed, 37 insertions(+), 3 deletions(-)

diff --git a/doc/source/sw-description.rst b/doc/source/sw-description.rst
index 3de3636..72a5cb8 100644
--- a/doc/source/sw-description.rst
+++ b/doc/source/sw-description.rst
@@ -738,6 +738,17 @@ As a general rule, swupdate doesn't copy out a file if the destination path

doesn't exists. This behavior could be changed using the special property
"create-destination".

+As another general rule, the raw file handler installs the file directly to the
+specified path. If the target file already exists and the raw file handler
+is interrupted, the existing file may be replaced by an empty or partially
+written file. A use case can exist where having an empty or corrupted file is
+worse than the existing file. For this reason, the raw file handler supports an
+"atomic-install" property. Setting the property to "true" installs the file to
+the specified path with ".tmp" apppended to the filename. Once the contents of
+the file have been written and the buffer is flushed, the ".tmp" file is renamed
+to the target file. This minimizes chances that an empty or corrupted file is
+created by an interrupted raw file handler.

if (strtobool(dict_get_value(&img->properties, "create-destination"))) {
TRACE("Creating path %s", path);
@@ -249,7 +258,7 @@ static int install_raw_file(struct img_type *img,
}
}

- fdout = openfileoutput(path);
+ fdout = openfileoutput(tmp_path);
if (fdout < 0)
return fdout;
if (!img_check_free_space(img, fdout)) {
@@ -260,8 +269,22 @@ static int install_raw_file(struct img_type *img,
if (ret< 0) {
ERROR("Error copying extracted file");
}
+
+ if (strtobool(dict_get_value(&img->properties, "atomic-install"))) {
+ if(fsync(fdout)) {
+ ERROR("Error writing %s to disk: %s", tmp_path, strerror(errno));
+ }
+ }
+

[Colin McAllister]

Hi Stefano,

I updated the patch with a new commit message and also updated my patch's addition to the docs.  Please let me know if the patch is acceptable when you have a moment.

Thanks,

Colin

[Stefano Babic]

On 09.02.22 18:41, 'Colin McAllister' via swupdate wrote:
> Hi Stefano,
>
> I updated the patch with a new commit message and also updated my
> patch's addition to the docs.  Please let me know if the patch is
> acceptable when you have a moment.

It is ok, I will review and check some details, but I am definetly going
to merge this feature.

Regards,
Stefano

> --
> You received this message because you are subscribed to the Google
> Groups "swupdate" group.
> To unsubscribe from this group and stop receiving emails from it, send
> an email to swupdate+u...@googlegroups.com
> <mailto:swupdate+u...@googlegroups.com>.
> To view this discussion on the web visit

> https://groups.google.com/d/msgid/swupdate/7cd5ec2e-23b2-4155-91bc-8b4c3e75247an%40googlegroups.com
> <https://groups.google.com/d/msgid/swupdate/7cd5ec2e-23b2-4155-91bc-8b4c3e75247an%40googlegroups.com?utm_medium=email&utm_source=footer>.

[Stefano Babic]

Hi Colin,

Taking into account Christian's comments to previous version of the
patch, do we need to call fsync() just in case "atomic-install" is
called ? Can we simply call it in any case before close() ?

> + ERROR("Error writing %s to disk: %s", tmp_path, strerror(errno));
> + }
> + }
> +
> close(fdout);
>
> + if (strtobool(dict_get_value(&img->properties, "atomic-install"))) {
> + TRACE("Renaming file %s to %s", tmp_path, path);
> + if(rename(tmp_path, path)) {
> + ERROR("Error renaming %s to %s: %s", tmp_path, path, strerror(errno));
> + }
> + }
> +
> if (use_mount) {
> swupdate_umount(DATADST_DIR);
> }

Best regards,
Stefano

[Colin McAllister]

Hi Stefano,

Yes, this could be done.  I opted to make the fsync conditional as it can be a time costly operation.

A less costly alternative would be a separate patch where sync(2) is called at the end of the update process.

I can push up a v5 patch with an unconditional fsync call, just let me know what you think is best.

[Colin McAllister]

Add property for raw file handler that installs file to .tmp file. Once
the .tmp file is installed and the write buffer is flushed, the file is
renamed to the specified file in path. This minimizes the chance of a
corrupted file if the raw file handler is interrupted.

Signed-off-by: Colin McAllister <colin.mc...@garmin.com>
---
doc/source/sw-description.rst | 11 +++++++++++

handlers/raw_handler.c | 27 ++++++++++++++++++++++++---
2 files changed, 35 insertions(+), 3 deletions(-)

diff --git a/doc/source/sw-description.rst b/doc/source/sw-description.rst
index 3de3636..72a5cb8 100644
--- a/doc/source/sw-description.rst
+++ b/doc/source/sw-description.rst
@@ -738,6 +738,17 @@ As a general rule, swupdate doesn't copy out a file if the destination path
doesn't exists. This behavior could be changed using the special property
"create-destination".

+As another general rule, the raw file handler installs the file directly to the
+specified path. If the target file already exists and the raw file handler
+is interrupted, the existing file may be replaced by an empty or partially
+written file. A use case can exist where having an empty or corrupted file is
+worse than the existing file. For this reason, the raw file handler supports an
+"atomic-install" property. Setting the property to "true" installs the file to
+the specified path with ".tmp" apppended to the filename. Once the contents of
+the file have been written and the buffer is flushed, the ".tmp" file is renamed
+to the target file. This minimizes chances that an empty or corrupted file is
+created by an interrupted raw file handler.
+
Scripts
-------

diff --git a/handlers/raw_handler.c b/handlers/raw_handler.c

index 619a2f4..d59b8c6 100644

@@ -260,8 +269,20 @@ static int install_raw_file(struct img_type *img,

if (ret< 0) {
ERROR("Error copying extracted file");
}
+

+ if(fsync(fdout)) {
+ ERROR("Error writing %s to disk: %s", tmp_path, strerror(errno));
+ }
+

[Stefano Babic]

Hi Colin,

I have still a couple of open questions:

Understood we need fsync - but an error is ignored. Is this a fatal
error, isn't it ? That means we should retun with an error code, right ?

> + }
> +
> close(fdout);
>
> + if (strtobool(dict_get_value(&img->properties, "atomic-install"))) {
> + TRACE("Renaming file %s to %s", tmp_path, path);
> + if(rename(tmp_path, path)) {
> + ERROR("Error renaming %s to %s: %s", tmp_path, path, strerror(errno));

I think this is worse. If just rename fails, and this is possible, it is
just tracked on the Log, but we finally get a filesystem with the ".tmp"
instead of the file we wanted, and the bad thing is that SWUpdate
returns with SUCCESS. Shouldn't we stop if rename fails, that is a
failure should be returned by the handler ?

> + }
> + }
> +
> if (use_mount) {
> swupdate_umount(DATADST_DIR);
> }

[Colin McAllister]

Add property for raw file handler that installs file to .tmp file. Once
the .tmp file is installed and the write buffer is flushed, the file is
renamed to the specified file in path. This minimizes the chance of a
corrupted file if the raw file handler is interrupted.

Signed-off-by: Colin McAllister <colin.mc...@garmin.com>
---
doc/source/sw-description.rst | 11 +++++++++++

handlers/raw_handler.c | 29 ++++++++++++++++++++++++++---
2 files changed, 37 insertions(+), 3 deletions(-)

diff --git a/doc/source/sw-description.rst b/doc/source/sw-description.rst
index 3de3636..72a5cb8 100644
--- a/doc/source/sw-description.rst
+++ b/doc/source/sw-description.rst
@@ -738,6 +738,17 @@ As a general rule, swupdate doesn't copy out a file if the destination path
doesn't exists. This behavior could be changed using the special property
"create-destination".

+As another general rule, the raw file handler installs the file directly to the
+specified path. If the target file already exists and the raw file handler
+is interrupted, the existing file may be replaced by an empty or partially
+written file. A use case can exist where having an empty or corrupted file is
+worse than the existing file. For this reason, the raw file handler supports an
+"atomic-install" property. Setting the property to "true" installs the file to
+the specified path with ".tmp" apppended to the filename. Once the contents of
+the file have been written and the buffer is flushed, the ".tmp" file is renamed
+to the target file. This minimizes chances that an empty or corrupted file is
+created by an interrupted raw file handler.
+
Scripts
-------

diff --git a/handlers/raw_handler.c b/handlers/raw_handler.c

index 619a2f4..2dba412 100644

@@ -260,8 +269,22 @@ static int install_raw_file(struct img_type *img,

if (ret< 0) {
ERROR("Error copying extracted file");

Shouldn't there also be a return here? I would imagine if the copy
failed, the handler should return an error. Since this section is
outside fo the scope of this patch, I ignored it, but I would be happy
to submit another patch fixing this.

}
+
+ if(fsync(fdout)) {
+ ERROR("Error writing %s to disk: %s", tmp_path, strerror(errno));

+ return -1;
Added a return here.

+ }
+
close(fdout);

+ if (strtobool(dict_get_value(&img->properties, "atomic-install"))) {
+ TRACE("Renaming file %s to %s", tmp_path, path);
+ if(rename(tmp_path, path)) {
+ ERROR("Error renaming %s to %s: %s", tmp_path, path, strerror(errno));

+ return -1;
Added a return here as well.

[Stefano Babic]

Hi Colin,

thanks for fixing.

Reviewed-by: Stefano Babic <sba...@denx.de>

Best regards,
Stefano Babic

[Stefano Babic]

Hi Colin,

something slip into the patch and I missed to anser:

Yes, but it was - the handler is returning ret, that means a non-zero in
case of error.

Can you resubmit removing the lines above ?

Regards,
Stefano

[Colin McAllister]

Add property for raw file handler that installs file to .tmp file. Once
the .tmp file is installed and the write buffer is flushed, the file is
renamed to the specified file in path. This minimizes the chance of a
corrupted file if the raw file handler is interrupted.

Signed-off-by: Colin McAllister <colin.mc...@garmin.com>
---
doc/source/sw-description.rst | 11 +++++++++++

handlers/raw_handler.c | 29 ++++++++++++++++++++++++++---
2 files changed, 37 insertions(+), 3 deletions(-)

diff --git a/doc/source/sw-description.rst b/doc/source/sw-description.rst
index 3de3636..72a5cb8 100644
--- a/doc/source/sw-description.rst
+++ b/doc/source/sw-description.rst
@@ -738,6 +738,17 @@ As a general rule, swupdate doesn't copy out a file if the destination path
doesn't exists. This behavior could be changed using the special property
"create-destination".

+As another general rule, the raw file handler installs the file directly to the
+specified path. If the target file already exists and the raw file handler
+is interrupted, the existing file may be replaced by an empty or partially
+written file. A use case can exist where having an empty or corrupted file is
+worse than the existing file. For this reason, the raw file handler supports an
+"atomic-install" property. Setting the property to "true" installs the file to
+the specified path with ".tmp" apppended to the filename. Once the contents of
+the file have been written and the buffer is flushed, the ".tmp" file is renamed
+to the target file. This minimizes chances that an empty or corrupted file is
+created by an interrupted raw file handler.
+
Scripts
-------

diff --git a/handlers/raw_handler.c b/handlers/raw_handler.c

index 619a2f4..2dba412 100644

@@ -260,8 +269,22 @@ static int install_raw_file(struct img_type *img,

if (ret< 0) {
ERROR("Error copying extracted file");
}
+
+ if(fsync(fdout)) {
+ ERROR("Error writing %s to disk: %s", tmp_path, strerror(errno));

+ return -1;

+ }
+
close(fdout);

+ if (strtobool(dict_get_value(&img->properties, "atomic-install"))) {
+ TRACE("Renaming file %s to %s", tmp_path, path);
+ if(rename(tmp_path, path)) {
+ ERROR("Error renaming %s to %s: %s", tmp_path, path, strerror(errno));

+ return -1;
+ }
+ }
+