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.

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

DAOS-16822 dfs: follow on improvement for dfs_sys_mkdir_p #15569

Merged
merged 3 commits into from
Dec 10, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
69 changes: 24 additions & 45 deletions src/client/dfs/dfs_sys.c
Original file line number Diff line number Diff line change
Expand Up @@ -11,14 +11,14 @@
#include <linux/xattr.h>
#include <daos/common.h>
#include <daos/event.h>

#include <gurt/atomic.h>

#include <daos.h>
#include <daos_fs.h>

#include <daos_fs_sys.h>

#include "dfs_internal.h"

/** Number of entries for readdir */
#define DFS_SYS_NUM_DIRENTS 24

Expand Down Expand Up @@ -1399,68 +1399,47 @@ dfs_sys_mkdir(dfs_sys_t *dfs_sys, const char *dir, mode_t mode,
return rc;
}

static int
check_existing_dir(dfs_sys_t *dfs_sys, const char *dir_path)
{
struct stat st = {0};
int rc;

rc = dfs_sys_stat(dfs_sys, dir_path, 0, &st);
if (rc != 0) {
D_DEBUG(DB_TRACE, "failed to stat %s: (%d)\n", dir_path, rc);
return rc;
}

/* if it's not a directory, fail */
if (!S_ISDIR(st.st_mode))
return EEXIST;

/* if it is a directory, then it's not an error */
return 0;
}

int
dfs_sys_mkdir_p(dfs_sys_t *dfs_sys, const char *dir_path, mode_t mode, daos_oclass_id_t cid)
{
int path_len = strnlen(dir_path, PATH_MAX);
char *_path = NULL;
char *ptr = NULL;
int rc = 0;
size_t path_len;
char *_path = NULL;
char *sptr = NULL;
char *tok;
dfs_obj_t *parent;
int rc = 0;

if (dfs_sys == NULL)
return EINVAL;
if (dir_path == NULL)
return EINVAL;
if (dir_path[0] != '/')
return EINVAL;

path_len = strnlen(dir_path, PATH_MAX);
if (path_len == PATH_MAX)
return ENAMETOOLONG;

D_STRNDUP(_path, dir_path, path_len);
if (_path == NULL)
return ENOMEM;

parent = NULL;
/* iterate through the parent directories and create them if necessary */
for (ptr = _path + 1; *ptr != '\0'; ptr++) {
if (*ptr != '/')
continue;
for (tok = strtok_r(_path, "/", &sptr); tok != NULL; tok = strtok_r(NULL, "/", &sptr)) {
dfs_obj_t *cur;

/* truncate the string here to create the parent */
*ptr = '\0';
rc = dfs_sys_mkdir(dfs_sys, _path, mode, cid);
if (rc != 0) {
if (rc != EEXIST)
D_GOTO(out_free, rc);
rc = check_existing_dir(dfs_sys, _path);
if (rc != 0)
D_GOTO(out_free, rc);
}
/* reset to keep going */
*ptr = '/';
rc = dfs_open(dfs_sys->dfs, parent, tok, mode | S_IFDIR, O_RDWR | O_CREAT, cid, 0,
daltonbohning marked this conversation as resolved.
Show resolved Hide resolved
NULL, &cur);
if (rc != 0)
D_GOTO(out_free, rc);
if (parent)
dfs_release(parent);
parent = cur;
}

/* create the final directory */
rc = dfs_sys_mkdir(dfs_sys, _path, mode, cid);
if (rc == EEXIST)
rc = check_existing_dir(dfs_sys, _path);
if (parent)
dfs_release(parent);

out_free:
D_FREE(_path);
Expand Down
10 changes: 9 additions & 1 deletion src/tests/suite/dfs_sys_unit_test.c
Original file line number Diff line number Diff line change
Expand Up @@ -848,6 +848,10 @@ dfs_sys_test_mkdir(void **state)
rc = dfs_sys_mkdir(dfs_sys_mt, file, S_IWUSR | S_IRUSR, 0);
assert_int_equal(rc, EEXIST);

rc = dfs_sys_remove(dfs_sys_mt, file, true, NULL);
assert_int_equal(rc, 0);
rc = dfs_sys_remove(dfs_sys_mt, child, true, NULL);
assert_int_equal(rc, 0);
rc = dfs_sys_remove(dfs_sys_mt, parent, true, NULL);
assert_int_equal(rc, 0);
}
Expand Down Expand Up @@ -879,8 +883,12 @@ dfs_sys_test_mkdir_p(void **state)

/* this shouldn't work */
rc = dfs_sys_mkdir_p(dfs_sys_mt, file, S_IWUSR | S_IRUSR, 0);
assert_int_equal(rc, EEXIST);
assert_int_equal(rc, ENOTDIR);
Copy link
Contributor

Choose a reason for hiding this comment

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

This was intentionally returning EEXIST before to be inline with this

$ mkdir dir
$ touch file
$ mkdir dir
mkdir: cannot create directory ‘dir’: File exists
$ mkdir file
mkdir: cannot create directory ‘file’: File exists
$ mkdir -p file
mkdir: cannot create directory ‘file’: File exists

And the C API - ENOTDIR is only when a parent path does not exist
https://linux.die.net/man/3/mkdir

EEXIST
The named file exists.
ENOTDIR
A component of the path prefix is not a directory.

Thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i should be able to change ENOTDIR To EEXISTS for the last lookup of the leaf entry. will update


rc = dfs_sys_remove(dfs_sys_mt, file, true, NULL);
assert_int_equal(rc, 0);
rc = dfs_sys_remove(dfs_sys_mt, child, true, NULL);
assert_int_equal(rc, 0);
rc = dfs_sys_remove(dfs_sys_mt, parent, true, NULL);
assert_int_equal(rc, 0);
}
Expand Down
Loading