| Message ID | 20240731003708.3979149-1-dominique.martinet@atmark-techno.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Stefano Babic |
| Headers | show |
| Series | util: swupdate_remove_directory: remove error if target is missing | expand |
Hi Dominique, On 31.07.24 02:37, Dominique Martinet wrote: > commit a6ad222f58a5 ("util: Ensure swupdate_remove_directory unmounts > path") added a check for swupdate_remove_directory() to umount the > target if it is a mount point, but if it had already been cleaned up > then an error will now be printed. > > If the file is already missing there is just nothing to do, do not print > an error message. > > Cc: James Hilliard <james.hilliard1@gmail.com> > Signed-off-by: Dominique Martinet <dominique.martinet@atmark-techno.com> > --- > our update script was already removing the path, so this error started > showing up after upgrading; But in this case you are doing something that it is not expected. SWUpdate must create and remove the resources, including paths, and it is not expected that someone else is removing files that belong to SWUpdate. Anyway, cleanup is not an error, and return error from swupdate_remove_directory is not checked. Any entry in the log by calling this function should be marked with a level lower than ERROR. > I guess we could keep the error if parent is > missing and perhaps a trace if the dir itself is missing? > Happy to adjust the patch if these messages are deemed useful. But is this happening if nobody is removing path or resources ? SWUpdate tracks what has created and destroy at the end. The ERROR you see are signalling that something happened that was unexpected (i.e. someone removing paths). I will lower down ERROR to WARN. > > (I guess I could also remove the cleanup on our side, IMHO it is better > I'm taking care to > not descend into submounts but since this runs afterwards anyway it's not > like that's any help; do we want to set FTW_MOUNT to the nftw call?) We can add to enforce and avoid side effect - SWUpdate is checking if it is a mount point and unmount itself. > > core/util.c | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff --git a/core/util.c b/core/util.c > index 33a9af7114a3..09bdf93b1e51 100644 > --- a/core/util.c > +++ b/core/util.c > @@ -173,12 +173,10 @@ static int _is_mount_point(const char *path, const char *parent_path) { > struct stat path_stat, parent_stat; > > if (stat(path, &path_stat)) { > - ERROR("stat for path %s failed: %s", path, strerror(errno)); > return -errno; > } > > if (stat(parent_path, &parent_stat)) { > - ERROR("stat for parent path %s failed: %s", parent_path, strerror(errno)); > return -errno; > } > > @@ -209,8 +207,11 @@ int swupdate_remove_directory(const char* path) > } > > ret = _is_mount_point(dpath, get_tmpdir()); > - if (ret < 0) > + if (ret < 0) { > + // directory already removed > + ret = 0; > goto out; > + } > > if (ret) { > WARN("Unexpected mountpoint, unmounting: %s", dpath); Best regards, Stefano
Stefano Babic wrote on Wed, Jul 31, 2024 at 09:46:52AM +0200: > > I guess we could keep the error if parent is > > missing and perhaps a trace if the dir itself is missing? > > Happy to adjust the patch if these messages are deemed useful. > > But is this happening if nobody is removing path or resources ? SWUpdate > tracks what has created and destroy at the end. The ERROR you see are > signalling that something happened that was unexpected (i.e. someone > removing paths). I will lower down ERROR to WARN. Thanks, I'll let you lower the warning (and perhaps fix the return value or make it void since nobody checks it? I guess that doesn't really matter) > > (I guess I could also remove the cleanup on our side, > > IMHO it is better Ok, I think that was leftover from before swupdate was cleaning it up, but looking back we don't have any release with that so it must have been early experiments; I'll just remove this cleanup on my side. > > I'm taking care to > > not descend into submounts but since this runs afterwards anyway it's not > > like that's any help; do we want to set FTW_MOUNT to the nftw call?) > > We can add to enforce and avoid side effect - SWUpdate is checking if it > is a mount point and unmount itself. (upon verification I actually wasn't taking any care at all there...) I'll note this is welcome; actually checking if something is a mountpoint is more work than I can put in right now but that might be an improvement for later. BTW note the current check for is a mountpoint is not correct in case of bind mount within the same filesystem, e.g. if /foo and /bar are in the same filesystem then `mount --bind /foo /bar/subdir` will not have /bar/subdir detected as a mountpoint by this version of _is_mount_point. The correct thing to do is to check if any mount targets in /proc/self/mounts is a subpath of the path we're considering (assuming it is canonical e.g. no ../ or double slashes), but that's also more work than I'm willing to put into short term as we never mount anything there so it's probably fine as is -- remove will fail, terminating nftw with -1 in turn and we're not checking that at all for a warning, but hopefully such mount points shouldn't happen... (but I guess that since James added the check it did somewhere?!) Cheers,
diff --git a/core/util.c b/core/util.c index 33a9af7114a3..09bdf93b1e51 100644 --- a/core/util.c +++ b/core/util.c @@ -173,12 +173,10 @@ static int _is_mount_point(const char *path, const char *parent_path) { struct stat path_stat, parent_stat; if (stat(path, &path_stat)) { - ERROR("stat for path %s failed: %s", path, strerror(errno)); return -errno; } if (stat(parent_path, &parent_stat)) { - ERROR("stat for parent path %s failed: %s", parent_path, strerror(errno)); return -errno; } @@ -209,8 +207,11 @@ int swupdate_remove_directory(const char* path) } ret = _is_mount_point(dpath, get_tmpdir()); - if (ret < 0) + if (ret < 0) { + // directory already removed + ret = 0; goto out; + } if (ret) { WARN("Unexpected mountpoint, unmounting: %s", dpath);
commit a6ad222f58a5 ("util: Ensure swupdate_remove_directory unmounts path") added a check for swupdate_remove_directory() to umount the target if it is a mount point, but if it had already been cleaned up then an error will now be printed. If the file is already missing there is just nothing to do, do not print an error message. Cc: James Hilliard <james.hilliard1@gmail.com> Signed-off-by: Dominique Martinet <dominique.martinet@atmark-techno.com> --- our update script was already removing the path, so this error started showing up after upgrading; I guess we could keep the error if parent is missing and perhaps a trace if the dir itself is missing? Happy to adjust the patch if these messages are deemed useful. (I guess I could also remove the cleanup on our side, I'm taking care to not descend into submounts but since this runs afterwards anyway it's not like that's any help; do we want to set FTW_MOUNT to the nftw call?) core/util.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)