From 33acfd0dcdf2a14b89118e245dcd9e0ca09ff976 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rapha=C3=ABl=20Proust?= Date: Sat, 1 Jul 2017 17:29:03 +0100 Subject: [PATCH 01/10] Remove unneeded macros GET_INET Fixes #426 --- src/unix/lwt_unix_unix.h | 5 ----- 1 file changed, 5 deletions(-) diff --git a/src/unix/lwt_unix_unix.h b/src/unix/lwt_unix_unix.h index 5559aebfcb..4510d3e7bc 100644 --- a/src/unix/lwt_unix_unix.h +++ b/src/unix/lwt_unix_unix.h @@ -841,8 +841,6 @@ CAMLprim value lwt_unix_mcast_set_ttl(value fd, value ttl) #define VAL_MCAST_ACTION_ADD (Val_int(0)) #define VAL_MCAST_ACTION_DROP (Val_int(1)) -#define GET_INET_ADDR(v) (*((struct in_addr *)(v))) - CAMLprim value lwt_unix_mcast_modify_membership(value fd, value v_action, value if_addr, value group_addr) { @@ -2364,10 +2362,7 @@ struct job_gethostbyname { }; CAMLexport value alloc_inet_addr(struct in_addr *inaddr); -#define GET_INET_ADDR(v) (*((struct in_addr *)(v))) - CAMLexport value alloc_inet6_addr(struct in6_addr *inaddr); -#define GET_INET6_ADDR(v) (*((struct in6_addr *)(v))) static value alloc_one_addr(char const *a) { From e521527807358852916631c4ac0dd117ab42abe9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rapha=C3=ABl=20Proust?= Date: Sat, 1 Jul 2017 19:04:40 +0100 Subject: [PATCH 02/10] Using `readdir` instead of deprecated `readdir_r` --- src/unix/lwt_unix_unix.h | 30 ++++++++++++++++++------------ 1 file changed, 18 insertions(+), 12 deletions(-) diff --git a/src/unix/lwt_unix_unix.h b/src/unix/lwt_unix_unix.h index 4510d3e7bc..236781d1d5 100644 --- a/src/unix/lwt_unix_unix.h +++ b/src/unix/lwt_unix_unix.h @@ -32,6 +32,7 @@ #include #include +#include #include #include #include @@ -1777,30 +1778,35 @@ struct job_readdir { struct lwt_unix_job job; DIR *dir; struct dirent *entry; - struct dirent *ptr; - int result; + int error_code; }; static void worker_readdir(struct job_readdir *job) { - job->entry = lwt_unix_malloc(dirent_size(job->dir)); - job->result = readdir_r(job->dir, job->entry, &job->ptr); + // From the man page of readdir + // If the end of the directory stream is reached, NULL is returned and + // errno is not changed. If an error occurs, NULL is returned and errno + // is set appropriately. To distinguish end of stream and from an error, + // set errno to zero before calling readdir() and then check the value of + // errno if NULL is returned. + errno = 0; + job->entry = readdir(job->dir); + job->error_code = errno; } static value result_readdir(struct job_readdir *job) { - int result = job->result; - if (result) { - free(job->entry); - lwt_unix_free_job(&job->job); - unix_error(result, "readdir", Nothing); - } else if (job->ptr == NULL) { - free(job->entry); + LWT_UNIX_CHECK_JOB(job, job->entry == NULL && job->error_code != 0, "readdir"); + if (job->entry == NULL) { + // From the man page + // On success, readdir() returns a pointer to a dirent structure. + // (This structure may be statically allocated; do not attempt to + // free(3) it.) lwt_unix_free_job(&job->job); caml_raise_end_of_file(); } else { value name = caml_copy_string(job->entry->d_name); - free(job->entry); + // see above about not freeing dirent lwt_unix_free_job(&job->job); return name; } From 12ececc4300d77897f9c8c7568146501f1198c91 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rapha=C3=ABl=20Proust?= Date: Sat, 1 Jul 2017 19:21:03 +0100 Subject: [PATCH 03/10] Replace deprecated `readdir_r` by `readdir` --- src/unix/lwt_unix_unix.h | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/src/unix/lwt_unix_unix.h b/src/unix/lwt_unix_unix.h index 236781d1d5..8df1b55d3c 100644 --- a/src/unix/lwt_unix_unix.h +++ b/src/unix/lwt_unix_unix.h @@ -1835,19 +1835,21 @@ static void worker_readdir_n(struct job_readdir_n *job) { long i; for (i = 0; i < job->count; i++) { - struct dirent *ptr; - int result = readdir_r(job->dir, &job->entries[i], &ptr); + errno = 0; + struct dirent *entry = readdir(job->dir); /* An error happened. */ - if (result != 0) { - job->error_code = result; + if (entry == NULL && errno != 0) { + job->error_code = errno; return; } /* End of directory reached */ - if (ptr == NULL) break; - } + if (entry == NULL && errno == 0) break; + /* All is good */ + job->entries[i] = *entry; + } job->count = i; job->error_code = 0; } From 69cc188ae4e526302102db163451b24d661bb299 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rapha=C3=ABl=20Proust?= Date: Sat, 1 Jul 2017 19:23:44 +0100 Subject: [PATCH 04/10] Remove support function for removed `readdir_r` --- src/unix/lwt_unix_unix.h | 17 +---------------- 1 file changed, 1 insertion(+), 16 deletions(-) diff --git a/src/unix/lwt_unix_unix.h b/src/unix/lwt_unix_unix.h index 8df1b55d3c..3d68e23a24 100644 --- a/src/unix/lwt_unix_unix.h +++ b/src/unix/lwt_unix_unix.h @@ -1754,21 +1754,6 @@ CAMLprim value lwt_unix_rewinddir_job(value dir) return lwt_unix_alloc_job(&(job->job)); } -/* struct dirent size */ - -/* Some kind of estimate of the true size of a dirent structure, including the - space used for the name. This is controversial, and there is an ongoing - discussion (see Internet) about deprecating readdir_r because of the need to - guess the size in this way. */ -static size_t dirent_size(DIR *dir) -{ - size_t size = offsetof(struct dirent, d_name) + - fpathconf(dirfd(dir), _PC_NAME_MAX) + 1; - - if (size < sizeof(struct dirent)) size = sizeof(struct dirent); - - return size; -} /* +-----------------------------------------------------------------+ | JOB: readdir | @@ -1878,7 +1863,7 @@ CAMLprim value lwt_unix_readdir_n_job(value val_dir, value val_count) long count = Long_val(val_count); DIR *dir = DIR_Val(val_dir); - LWT_UNIX_INIT_JOB(job, readdir_n, dirent_size(dir) * count); + LWT_UNIX_INIT_JOB(job, readdir_n, sizeof(struct dirent) * count); job->dir = dir; job->count = count; From e61777e8bb7a32d097efbe977d6c8a87fee2bee5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rapha=C3=ABl=20Proust?= Date: Sat, 1 Jul 2017 19:28:45 +0100 Subject: [PATCH 05/10] clang-format recently modified C file --- src/unix/lwt_unix_unix.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/unix/lwt_unix_unix.h b/src/unix/lwt_unix_unix.h index 3d68e23a24..0337458e10 100644 --- a/src/unix/lwt_unix_unix.h +++ b/src/unix/lwt_unix_unix.h @@ -1754,7 +1754,6 @@ CAMLprim value lwt_unix_rewinddir_job(value dir) return lwt_unix_alloc_job(&(job->job)); } - /* +-----------------------------------------------------------------+ | JOB: readdir | +-----------------------------------------------------------------+ */ @@ -1781,7 +1780,8 @@ static void worker_readdir(struct job_readdir *job) static value result_readdir(struct job_readdir *job) { - LWT_UNIX_CHECK_JOB(job, job->entry == NULL && job->error_code != 0, "readdir"); + LWT_UNIX_CHECK_JOB(job, job->entry == NULL && job->error_code != 0, + "readdir"); if (job->entry == NULL) { // From the man page // On success, readdir() returns a pointer to a dirent structure. From 4ba200ae38caf7ca489988d9515d131c2a1a6806 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rapha=C3=ABl=20Proust?= Date: Mon, 3 Jul 2017 22:44:30 +0100 Subject: [PATCH 06/10] Copy directory names from dirent in readdir_n --- src/unix/lwt_unix_unix.h | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/unix/lwt_unix_unix.h b/src/unix/lwt_unix_unix.h index 0337458e10..a57cf9acd0 100644 --- a/src/unix/lwt_unix_unix.h +++ b/src/unix/lwt_unix_unix.h @@ -1813,7 +1813,7 @@ struct job_readdir_n { DIR *dir; long count; int error_code; - struct dirent entries[]; + char *entries[]; }; static void worker_readdir_n(struct job_readdir_n *job) @@ -1833,7 +1833,7 @@ static void worker_readdir_n(struct job_readdir_n *job) if (entry == NULL && errno == 0) break; /* All is good */ - job->entries[i] = *entry; + job->entries[i] = strdup(entry->d_name); } job->count = i; job->error_code = 0; @@ -1851,7 +1851,7 @@ static value result_readdir_n(struct job_readdir_n *job) result = caml_alloc(job->count, 0); long i; for (i = 0; i < job->count; i++) { - Store_field(result, i, caml_copy_string(job->entries[i].d_name)); + Store_field(result, i, caml_copy_string(job->entries[i])); } lwt_unix_free_job(&job->job); CAMLreturn(result); From 0dd40d85ab531609ad55eee4b984fc552794956e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rapha=C3=ABl=20Proust?= Date: Tue, 4 Jul 2017 07:45:15 +0100 Subject: [PATCH 07/10] More thorough cleanup of memory in readdir --- src/unix/lwt_unix_unix.h | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/src/unix/lwt_unix_unix.h b/src/unix/lwt_unix_unix.h index a57cf9acd0..3268d25e1b 100644 --- a/src/unix/lwt_unix_unix.h +++ b/src/unix/lwt_unix_unix.h @@ -1826,12 +1826,21 @@ static void worker_readdir_n(struct job_readdir_n *job) /* An error happened. */ if (entry == NULL && errno != 0) { job->error_code = errno; + job->count = i - 1; return; } /* End of directory reached */ if (entry == NULL && errno == 0) break; + /* readdir is good */ + char *name = strdup(entry->d_name); + if (name == NULL) { + job->error_code = errno; + job->count = i - 1; + return; + } + /* All is good */ job->entries[i] = strdup(entry->d_name); } @@ -1845,6 +1854,8 @@ static value result_readdir_n(struct job_readdir_n *job) CAMLlocal1(result); int error_code = job->error_code; if (error_code) { + long i; + for (i = 0; i < job->count; i++) free(job->entries[i]); lwt_unix_free_job(&job->job); unix_error(error_code, "readdir", Nothing); } else { @@ -1853,6 +1864,7 @@ static value result_readdir_n(struct job_readdir_n *job) for (i = 0; i < job->count; i++) { Store_field(result, i, caml_copy_string(job->entries[i])); } + for (i = 0; i < job->count; i++) free(job->entries[i]); lwt_unix_free_job(&job->job); CAMLreturn(result); } @@ -1863,7 +1875,7 @@ CAMLprim value lwt_unix_readdir_n_job(value val_dir, value val_count) long count = Long_val(val_count); DIR *dir = DIR_Val(val_dir); - LWT_UNIX_INIT_JOB(job, readdir_n, sizeof(struct dirent) * count); + LWT_UNIX_INIT_JOB(job, readdir_n, sizeof(char *) * count); job->dir = dir; job->count = count; From b7214cf39bfac2e2d1d84440fdf6f368a4b3b997 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rapha=C3=ABl=20Proust?= Date: Tue, 4 Jul 2017 07:51:20 +0100 Subject: [PATCH 08/10] Comment readdir_n's double use of count --- src/unix/lwt_unix_unix.h | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/unix/lwt_unix_unix.h b/src/unix/lwt_unix_unix.h index 3268d25e1b..856114b752 100644 --- a/src/unix/lwt_unix_unix.h +++ b/src/unix/lwt_unix_unix.h @@ -1811,6 +1811,13 @@ CAMLprim value lwt_unix_readdir_job(value val_dir) struct job_readdir_n { struct lwt_unix_job job; DIR *dir; + /* count serves two purpose: + 1. Transmit the maximum number of entries requested by the programmer. + See `readdir_n`'s OCaml side documentation. + 2. Transmit the number of actually read entries from the `worker` to + the result parser. + See below `worker_readdir_n` and `result_readdir_n` + */ long count; int error_code; char *entries[]; From 8ba3489f26550dfafb316d5f9a12720de2bb9d49 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rapha=C3=ABl=20Proust?= Date: Tue, 4 Jul 2017 20:37:05 +0100 Subject: [PATCH 09/10] Free all read dirnames in readdir_n --- src/unix/lwt_unix_unix.h | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/src/unix/lwt_unix_unix.h b/src/unix/lwt_unix_unix.h index 856114b752..26c0e43e42 100644 --- a/src/unix/lwt_unix_unix.h +++ b/src/unix/lwt_unix_unix.h @@ -1832,8 +1832,8 @@ static void worker_readdir_n(struct job_readdir_n *job) /* An error happened. */ if (entry == NULL && errno != 0) { + job->count = i; job->error_code = errno; - job->count = i - 1; return; } @@ -1843,8 +1843,8 @@ static void worker_readdir_n(struct job_readdir_n *job) /* readdir is good */ char *name = strdup(entry->d_name); if (name == NULL) { + job->count = i; job->error_code = errno; - job->count = i - 1; return; } @@ -3199,7 +3199,12 @@ CAMLprim value lwt_unix_getcwd_job(value unit) The last argument is the number of bytes of storage to reserve in memory immediately following the `struct`. This is for fields such as `char data[]` at the end of the struct. It is typically zero. For an - example where it is not zero, see `lwt_unix_read_job` again. */ + example where it is not zero, see `lwt_unix_read_job` again. + + If the additional data is stored inline in the job struct, it is + deallocated with `lwt_unix_free_job`. If the additional data is for + pointers to additional structure, you must remember to deallocate it + yourself. For an example of this, see `readdir_n`.*/ LWT_UNIX_INIT_JOB(job, getcwd, 0); /* Allocate a corresponding object in the OCaml heap. `&job->job` is the From 4d19c3f1476d4456cc38650f6930ca34a59fc0da Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rapha=C3=ABl=20Proust?= Date: Tue, 4 Jul 2017 21:59:21 +0100 Subject: [PATCH 10/10] Assign the result of srtdup not another call --- src/unix/lwt_unix_unix.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/unix/lwt_unix_unix.h b/src/unix/lwt_unix_unix.h index 26c0e43e42..0313c1cfeb 100644 --- a/src/unix/lwt_unix_unix.h +++ b/src/unix/lwt_unix_unix.h @@ -1849,7 +1849,7 @@ static void worker_readdir_n(struct job_readdir_n *job) } /* All is good */ - job->entries[i] = strdup(entry->d_name); + job->entries[i] = name; } job->count = i; job->error_code = 0;