libxl: avoid infinite loop in libxl__remove_directory()

Commit Message

Jan Beulich March 6, 2025, 11:25 a.m. UTC

Infinitely retrying the rmdir() invocation makes little sense. While the
original observation was the log filling the disk (due to repeated
"Directory not empty" errors, in turn occurring for unclear reasons),
the loop wants breaking even if there was no error message being logged
(much like is done in the similar loops in libxl__remove_file() and
libxl__remove_file_or_directory()).

Fixes: c4dcbee67e6d ("libxl: provide libxl__remove_file et al")
Signed-off-by: Jan Beulich <[e-mail]>
---
This is the simplest possible adjustment. Depending on why there were
retries, retrying a few times may make sense. But then, especially for
the specific error observed, presumably only after having tried to empty
the directory another time.

It's further questionable whether trying the rmdir() makes sense at all
when emptying the directory failed. After all failure of opendir() also
results in bailing from the function without trying to rmdir(). If this
makes sense, then I further think that "rc" would want resetting ahead
of this final loop in the function: If the rmdir() succeeds despite
earlier errors, all is (kind of) fine.

Comments

Juergen Gross March 6, 2025, 3:01 p.m. UTC | #1

On 06.03.25 12:25, Jan Beulich wrote:
> Infinitely retrying the rmdir() invocation makes little sense. While the
> original observation was the log filling the disk (due to repeated
> "Directory not empty" errors, in turn occurring for unclear reasons),
> the loop wants breaking even if there was no error message being logged
> (much like is done in the similar loops in libxl__remove_file() and
> libxl__remove_file_or_directory()).
> 
> Fixes: c4dcbee67e6d ("libxl: provide libxl__remove_file et al")
> Signed-off-by: Jan Beulich <[e-mail]>

While probably a more sane solution by reworking this whole mess
is possible, this patch should solve the issue Jan has described.

So:

Reviewed-by: Juergen Gross <[e-mail]>


Juergen

Anthony PERARD March 12, 2025, 5:02 p.m. UTC | #2

On Thu, Mar 06, 2025 at 04:01:53PM +0100, Jürgen Groß wrote:
> On 06.03.25 12:25, Jan Beulich wrote:
> > Infinitely retrying the rmdir() invocation makes little sense. While the
> > original observation was the log filling the disk (due to repeated
> > "Directory not empty" errors, in turn occurring for unclear reasons),
> > the loop wants breaking even if there was no error message being logged
> > (much like is done in the similar loops in libxl__remove_file() and
> > libxl__remove_file_or_directory()).
> > 
> > Fixes: c4dcbee67e6d ("libxl: provide libxl__remove_file et al")
> > Signed-off-by: Jan Beulich <[e-mail]>
> 
> While probably a more sane solution by reworking this whole mess

libxl?

> is possible, this patch should solve the issue Jan has described.
> 
> So:
> 
> Reviewed-by: Juergen Gross <[e-mail]>

Acked-by: Anthony PERARD <[e-mail]>

Thanks,

Patch

--- a/tools/libs/light/libxl_utils.c
+++ b/tools/libs/light/libxl_utils.c
@@ -577,6 +577,7 @@  int libxl__remove_directory(libxl__gc *g
         if (errno == EINTR) continue;
         LOGE(ERROR, "failed to remove emptied directory %s", dirpath);
         rc = ERROR_FAIL;
+        break;
     }
 
  out: