Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add retry to storagePosixPathRemove(). #1786

Closed
wants to merge 3 commits into from
Closed

Add retry to storagePosixPathRemove(). #1786

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
fcc2b30
More robust impl.
dwsteele Jun 15, 2022
77b0c46
Update tests.
dwsteele Jun 15, 2022
bd4a9b5
More impl.
dwsteele Jun 15, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
58 changes: 40 additions & 18 deletions src/storage/posix/storage.c
View file
Open in desktop
Expand Up @@ -433,6 +433,17 @@ storagePosixPathRemoveCallback(void *const callbackData, const StorageInfo *cons
FUNCTION_TEST_RETURN_VOID();
}

// Helper to determine if an error code indicates that a path is not empty. This can vary by implementation.
static bool
storagePosixErrorIsEmpty(const int error)
{
FUNCTION_TEST_BEGIN();
FUNCTION_TEST_PARAM(INT, error);
FUNCTION_TEST_END();

FUNCTION_TEST_RETURN(BOOL, error == ENOTEMPTY || error == EEXIST);
}

static bool
storagePosixPathRemove(THIS_VOID, const String *path, bool recurse, StorageInterfacePathRemoveParam param)
{
Expand All @@ -452,28 +463,39 @@ storagePosixPathRemove(THIS_VOID, const String *path, bool recurse, StorageInter

MEM_CONTEXT_TEMP_BEGIN()
{
// Recurse if requested
if (recurse)
{
StoragePosixPathRemoveData data =
{
.driver = this,
.path = path,
};
bool removed = false;

// Remove all sub paths/files
storageInterfaceInfoListP(this, path, storageInfoLevelExists, storagePosixPathRemoveCallback, &data);
}

// Delete the path
if (rmdir(strZ(path)) == -1)
do
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would be a great place to leave a comment 1) keep trying until the directory is fully removed, and 2) why it might not work the first time.

Can we guarantee it can't go into an infinite loop? Perhaps a limit on how many times we retry?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment updated in 8fcb15c and b36d5af.

Can we guarantee it can't go into an infinite loop? Perhaps a limit on how many times we retry?

It seems unlikely, but not impossible. A retry limit would be a good here -- that's pretty standard practice elsewhere. That requires shimming something, but not that big a deal.

{
if (errno != ENOENT) // {vm_covered}
THROW_SYS_ERROR_FMT(PathRemoveError, STORAGE_ERROR_PATH_REMOVE, strZ(path)); // {vm_covered}
// If path could not be removed
if (rmdir(strZ(path)) == -1)
{
// Path does not exist
if (errno == ENOENT)
jreidthompson marked this conversation as resolved.
Show resolved Hide resolved
{
result = false;
removed = true;
}
// Else if not empty but recursion requested then remove all sub paths/files
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a bit confused by the comment and the name of the function. Should the function be named "storagePosixErrorIsNotEmpty"?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It certainly should. Fixed in 5c5f599.

else if (storagePosixErrorIsEmpty(errno) && recurse) // {vm_covered}
{
StoragePosixPathRemoveData data =
{
.driver = this,
.path = path,
};

// Path does not exist
result = false;
storageInterfaceInfoListP(this, path, storageInfoLevelExists, storagePosixPathRemoveCallback, &data);
}
// Else error
else // {vm_covered}
THROW_SYS_ERROR_FMT(PathRemoveError, STORAGE_ERROR_PATH_REMOVE, strZ(path)); // {vm_covered}
}
// Else path was removed
else
removed = true;
}
while (!removed);
}
MEM_CONTEXT_TEMP_END();

Expand Down
24 changes: 15 additions & 9 deletions test/src/module/storage/posixTest.c
View file
Open in desktop
Expand Up @@ -796,25 +796,24 @@ testRun(void)
storagePathRemoveP(storageTest, pathRemove2), PathRemoveError, STORAGE_ERROR_PATH_REMOVE ": [13] Permission denied",
strZ(pathRemove2));
TEST_ERROR_FMT(
storagePathRemoveP(storageTest, pathRemove2, .recurse = true), PathOpenError,
STORAGE_ERROR_LIST_INFO ": [13] Permission denied", strZ(pathRemove2));
storagePathRemoveP(storageTest, pathRemove2, .recurse = true), PathRemoveError,
STORAGE_ERROR_PATH_REMOVE ": [13] Permission denied", strZ(pathRemove2));

// -------------------------------------------------------------------------------------------------------------------------
TEST_TITLE("path remove - subpath permission denied");

HRN_SYSTEM_FMT("sudo chmod 777 %s", strZ(pathRemove1));
String *fileRemove = strNewFmt("%s/remove.txt", strZ(pathRemove2));

HRN_SYSTEM_FMT("sudo chmod 777 %s && sudo touch %s", strZ(pathRemove1), strZ(fileRemove));

TEST_ERROR_FMT(
storagePathRemoveP(storageTest, pathRemove2, .recurse = true), PathOpenError,
storagePathRemoveP(storageTest, pathRemove1, .recurse = true), PathOpenError,
STORAGE_ERROR_LIST_INFO ": [13] Permission denied", strZ(pathRemove2));

// -------------------------------------------------------------------------------------------------------------------------
TEST_TITLE("path remove - file in subpath, permission denied");

String *fileRemove = strNewFmt("%s/remove.txt", strZ(pathRemove2));

HRN_SYSTEM_FMT(
"sudo chmod 755 %s && sudo touch %s && sudo chmod 777 %s", strZ(pathRemove2), strZ(fileRemove), strZ(fileRemove));
HRN_SYSTEM_FMT("sudo chmod 755 %s && sudo chmod 777 %s", strZ(pathRemove2), strZ(fileRemove));

TEST_ERROR_FMT(
storagePathRemoveP(storageTest, pathRemove1, .recurse = true), PathRemoveError,
Expand All @@ -832,10 +831,17 @@ testRun(void)
#endif // TEST_CONTAINER_REQUIRED

// -------------------------------------------------------------------------------------------------------------------------
TEST_TITLE("path remove - path with subpath removed");
TEST_TITLE("path remove without recurse");

HRN_SYSTEM_FMT("mkdir -p %s", strZ(pathRemove2));

TEST_ERROR_FMT(
storagePathRemoveP(storageTest, pathRemove1), PathRemoveError,
STORAGE_ERROR_PATH_REMOVE ": [39] Directory not empty", strZ(pathRemove1));

// -------------------------------------------------------------------------------------------------------------------------
TEST_TITLE("path remove - path with subpath removed");

TEST_RESULT_VOID(
storagePathRemoveP(storageTest, pathRemove1, .recurse = true), "remove path");
TEST_RESULT_BOOL(
Expand Down