Praparation series to enable constification of struct ctl_table further
down the line.
No functional changes are intended.
These changes have been split out and reworked from my original
const sysctl patchset [0].
I'm resubmitting the patchset in smaller chunks for easier review.
Each split-out series is meant to be useful on its own.
Changes since the original series:
* Explicit initializartion of header->type in init_header()
* Some additional cleanups
[0] https://lore.kernel.org/lkml/[email protected]/
---
Thomas Weißschuh (4):
sysctl: drop sysctl_is_perm_empty_ctl_table
sysctl: move sysctl type to ctl_table_header
sysctl: drop now unnecessary out-of-bounds check
sysctl: remove unnecessary sentinel element
fs/proc/proc_sysctl.c | 19 ++++++++-----------
include/linux/sysctl.h | 22 +++++++++++-----------
2 files changed, 19 insertions(+), 22 deletions(-)
---
base-commit: b401b621758e46812da61fa58a67c3fd8d91de0d
change-id: 20231216-sysctl-empty-dir-71d7631f7bfe
Best regards,
--
Thomas Weißschuh <[email protected]>
As static initialization of the is not possible anymore move it into
init_header() where all the other header fields are also initialized.
Reduce memory consumption as there are less instances of
ctl_table_header than ctl_table.
Removing this mutable member also opens the way to constify static
instances of ctl_table.
Signed-off-by: Thomas Weißschuh <[email protected]>
---
fs/proc/proc_sysctl.c | 10 ++++++----
include/linux/sysctl.h | 22 +++++++++++-----------
2 files changed, 17 insertions(+), 15 deletions(-)
diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c
index 2f4d4329d83d..fde7a2f773f0 100644
--- a/fs/proc/proc_sysctl.c
+++ b/fs/proc/proc_sysctl.c
@@ -31,7 +31,7 @@ static const struct inode_operations proc_sys_dir_operations;
/* Support for permanently empty directories */
static struct ctl_table sysctl_mount_point[] = {
- {.type = SYSCTL_TABLE_TYPE_PERMANENTLY_EMPTY }
+ { }
};
/**
@@ -49,11 +49,11 @@ struct ctl_table_header *register_sysctl_mount_point(const char *path)
EXPORT_SYMBOL(register_sysctl_mount_point);
#define sysctl_is_perm_empty_ctl_header(hptr) \
- (hptr->ctl_table[0].type == SYSCTL_TABLE_TYPE_PERMANENTLY_EMPTY)
+ (hptr->type == SYSCTL_TABLE_TYPE_PERMANENTLY_EMPTY)
#define sysctl_set_perm_empty_ctl_header(hptr) \
- (hptr->ctl_table[0].type = SYSCTL_TABLE_TYPE_PERMANENTLY_EMPTY)
+ (hptr->type = SYSCTL_TABLE_TYPE_PERMANENTLY_EMPTY)
#define sysctl_clear_perm_empty_ctl_header(hptr) \
- (hptr->ctl_table[0].type = SYSCTL_TABLE_TYPE_DEFAULT)
+ (hptr->type = SYSCTL_TABLE_TYPE_DEFAULT)
void proc_sys_poll_notify(struct ctl_table_poll *poll)
{
@@ -208,6 +208,8 @@ static void init_header(struct ctl_table_header *head,
node++;
}
}
+ if (table == sysctl_mount_point)
+ sysctl_set_perm_empty_ctl_header(head);
}
static void erase_header(struct ctl_table_header *head)
diff --git a/include/linux/sysctl.h b/include/linux/sysctl.h
index ee7d33b89e9e..c87f73c06cb9 100644
--- a/include/linux/sysctl.h
+++ b/include/linux/sysctl.h
@@ -137,17 +137,6 @@ struct ctl_table {
void *data;
int maxlen;
umode_t mode;
- /**
- * enum type - Enumeration to differentiate between ctl target types
- * @SYSCTL_TABLE_TYPE_DEFAULT: ctl target with no special considerations
- * @SYSCTL_TABLE_TYPE_PERMANENTLY_EMPTY: Used to identify a permanently
- * empty directory target to serve
- * as mount point.
- */
- enum {
- SYSCTL_TABLE_TYPE_DEFAULT,
- SYSCTL_TABLE_TYPE_PERMANENTLY_EMPTY
- } type;
proc_handler *proc_handler; /* Callback for text formatting */
struct ctl_table_poll *poll;
void *extra1;
@@ -188,6 +177,17 @@ struct ctl_table_header {
struct ctl_dir *parent;
struct ctl_node *node;
struct hlist_head inodes; /* head for proc_inode->sysctl_inodes */
+ /**
+ * enum type - Enumeration to differentiate between ctl target types
+ * @SYSCTL_TABLE_TYPE_DEFAULT: ctl target with no special considerations
+ * @SYSCTL_TABLE_TYPE_PERMANENTLY_EMPTY: Used to identify a permanently
+ * empty directory target to serve
+ * as mount point.
+ */
+ enum {
+ SYSCTL_TABLE_TYPE_DEFAULT,
+ SYSCTL_TABLE_TYPE_PERMANENTLY_EMPTY,
+ } type;
};
struct ctl_dir {
--
2.43.2
It is used only twice and those callers are simpler with
sysctl_is_perm_empty_ctl_header().
So use this sibling function.
Signed-off-by: Thomas Weißschuh <[email protected]>
---
fs/proc/proc_sysctl.c | 8 +++-----
1 file changed, 3 insertions(+), 5 deletions(-)
diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c
index 37cde0efee57..2f4d4329d83d 100644
--- a/fs/proc/proc_sysctl.c
+++ b/fs/proc/proc_sysctl.c
@@ -48,10 +48,8 @@ struct ctl_table_header *register_sysctl_mount_point(const char *path)
}
EXPORT_SYMBOL(register_sysctl_mount_point);
-#define sysctl_is_perm_empty_ctl_table(tptr) \
- (tptr[0].type == SYSCTL_TABLE_TYPE_PERMANENTLY_EMPTY)
#define sysctl_is_perm_empty_ctl_header(hptr) \
- (sysctl_is_perm_empty_ctl_table(hptr->ctl_table))
+ (hptr->ctl_table[0].type == SYSCTL_TABLE_TYPE_PERMANENTLY_EMPTY)
#define sysctl_set_perm_empty_ctl_header(hptr) \
(hptr->ctl_table[0].type = SYSCTL_TABLE_TYPE_PERMANENTLY_EMPTY)
#define sysctl_clear_perm_empty_ctl_header(hptr) \
@@ -233,7 +231,7 @@ static int insert_header(struct ctl_dir *dir, struct ctl_table_header *header)
/* Am I creating a permanently empty directory? */
if (header->ctl_table_size > 0 &&
- sysctl_is_perm_empty_ctl_table(header->ctl_table)) {
+ sysctl_is_perm_empty_ctl_header(header)) {
if (!RB_EMPTY_ROOT(&dir->root))
return -EINVAL;
sysctl_set_perm_empty_ctl_header(dir_h);
@@ -1204,7 +1202,7 @@ static bool get_links(struct ctl_dir *dir,
struct ctl_table *entry, *link;
if (header->ctl_table_size == 0 ||
- sysctl_is_perm_empty_ctl_table(header->ctl_table))
+ sysctl_is_perm_empty_ctl_header(header))
return true;
/* Are there links available for every entry in table? */
--
2.43.2
The previous commits made this sentinel element unnecessary, remove it.
Signed-off-by: Thomas Weißschuh <[email protected]>
---
fs/proc/proc_sysctl.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c
index 4cdf98c6a9a4..7c0e27dc3d9d 100644
--- a/fs/proc/proc_sysctl.c
+++ b/fs/proc/proc_sysctl.c
@@ -30,9 +30,7 @@ static const struct file_operations proc_sys_dir_file_operations;
static const struct inode_operations proc_sys_dir_operations;
/* Support for permanently empty directories */
-static struct ctl_table sysctl_mount_point[] = {
- { }
-};
+static struct ctl_table sysctl_mount_point[] = { };
/**
* register_sysctl_mount_point() - registers a sysctl mount point
--
2.43.2
On Thu, Feb 22, 2024 at 08:07:35AM +0100, Thomas Wei?schuh wrote:
> Praparation series to enable constification of struct ctl_table further
> down the line.
> No functional changes are intended.
>
> These changes have been split out and reworked from my original
> const sysctl patchset [0].
> I'm resubmitting the patchset in smaller chunks for easier review.
> Each split-out series is meant to be useful on its own.
>
> Changes since the original series:
> * Explicit initializartion of header->type in init_header()
> * Some additional cleanups
>
> [0] https://lore.kernel.org/lkml/[email protected]/
This all looks super sexy to me, but I'd love Eric's feedback on patch
series before merging as he may know some other facts over sysctl_mount_point
I might be missing.
Reviewed-by: Luis Chamberlain <[email protected]>
Luis
On Thu, Feb 22, 2024 at 08:07:37AM +0100, Thomas Wei?schuh wrote:
> As static initialization of the is not possible anymore move it into
> init_header() where all the other header fields are also initialized.
Please say what was done. Something like: "Moved the
SYSCTL_TABLE_TYPE_{DEFAULT,PERMANENTLY_EMPTY} enumerations from
ctl_table to ctl_table_header."
And the then you can mention the why: "Removing the mutable memeber from
ctl_table opens the ...."
>
> Reduce memory consumption as there are less instances of
> ctl_table_header than ctl_table.
This is indeed true, but the main reasoning behind this is
constification. Right? If you want to leave this comment, I would
suggest you add something that talks about the amount of bytes saved.
Something like : "Reduce memory consumption by sizeof(int) for every
ctl_table entry in ctl_table_header". Or something similar.
>
> Removing this mutable member also opens the way to constify static
> instances of ctl_table.
>
> Signed-off-by: Thomas Wei?schuh <[email protected]>
> ---
> fs/proc/proc_sysctl.c | 10 ++++++----
> include/linux/sysctl.h | 22 +++++++++++-----------
> 2 files changed, 17 insertions(+), 15 deletions(-)
>
> diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c
> index 2f4d4329d83d..fde7a2f773f0 100644
> --- a/fs/proc/proc_sysctl.c
> +++ b/fs/proc/proc_sysctl.c
> @@ -31,7 +31,7 @@ static const struct inode_operations proc_sys_dir_operations;
>
> /* Support for permanently empty directories */
> static struct ctl_table sysctl_mount_point[] = {
> - {.type = SYSCTL_TABLE_TYPE_PERMANENTLY_EMPTY }
> + { }
> };
>
> /**
> @@ -49,11 +49,11 @@ struct ctl_table_header *register_sysctl_mount_point(const char *path)
> EXPORT_SYMBOL(register_sysctl_mount_point);
>
> #define sysctl_is_perm_empty_ctl_header(hptr) \
> - (hptr->ctl_table[0].type == SYSCTL_TABLE_TYPE_PERMANENTLY_EMPTY)
> + (hptr->type == SYSCTL_TABLE_TYPE_PERMANENTLY_EMPTY)
> #define sysctl_set_perm_empty_ctl_header(hptr) \
> - (hptr->ctl_table[0].type = SYSCTL_TABLE_TYPE_PERMANENTLY_EMPTY)
> + (hptr->type = SYSCTL_TABLE_TYPE_PERMANENTLY_EMPTY)
> #define sysctl_clear_perm_empty_ctl_header(hptr) \
> - (hptr->ctl_table[0].type = SYSCTL_TABLE_TYPE_DEFAULT)
> + (hptr->type = SYSCTL_TABLE_TYPE_DEFAULT)
>
> void proc_sys_poll_notify(struct ctl_table_poll *poll)
> {
> @@ -208,6 +208,8 @@ static void init_header(struct ctl_table_header *head,
> node++;
> }
> }
> + if (table == sysctl_mount_point)
> + sysctl_set_perm_empty_ctl_header(head);
> }
>
> static void erase_header(struct ctl_table_header *head)
> diff --git a/include/linux/sysctl.h b/include/linux/sysctl.h
> index ee7d33b89e9e..c87f73c06cb9 100644
> --- a/include/linux/sysctl.h
> +++ b/include/linux/sysctl.h
> @@ -137,17 +137,6 @@ struct ctl_table {
> void *data;
> int maxlen;
> umode_t mode;
> - /**
> - * enum type - Enumeration to differentiate between ctl target types
> - * @SYSCTL_TABLE_TYPE_DEFAULT: ctl target with no special considerations
> - * @SYSCTL_TABLE_TYPE_PERMANENTLY_EMPTY: Used to identify a permanently
> - * empty directory target to serve
> - * as mount point.
> - */
> - enum {
> - SYSCTL_TABLE_TYPE_DEFAULT,
> - SYSCTL_TABLE_TYPE_PERMANENTLY_EMPTY
> - } type;
> proc_handler *proc_handler; /* Callback for text formatting */
> struct ctl_table_poll *poll;
> void *extra1;
> @@ -188,6 +177,17 @@ struct ctl_table_header {
> struct ctl_dir *parent;
> struct ctl_node *node;
> struct hlist_head inodes; /* head for proc_inode->sysctl_inodes */
> + /**
> + * enum type - Enumeration to differentiate between ctl target types
> + * @SYSCTL_TABLE_TYPE_DEFAULT: ctl target with no special considerations
> + * @SYSCTL_TABLE_TYPE_PERMANENTLY_EMPTY: Used to identify a permanently
> + * empty directory target to serve
> + * as mount point.
> + */
> + enum {
> + SYSCTL_TABLE_TYPE_DEFAULT,
> + SYSCTL_TABLE_TYPE_PERMANENTLY_EMPTY,
> + } type;
> };
>
> struct ctl_dir {
>
> --
> 2.43.2
>
--
Joel Granados
On Thu, Feb 22, 2024 at 08:07:36AM +0100, Thomas Wei?schuh wrote:
> It is used only twice and those callers are simpler with
> sysctl_is_perm_empty_ctl_header().
> So use this sibling function.
Can you please add a comment relating this to the constification effort.
>
> Signed-off-by: Thomas Wei?schuh <[email protected]>
> ---
> fs/proc/proc_sysctl.c | 8 +++-----
> 1 file changed, 3 insertions(+), 5 deletions(-)
>
> diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c
> index 37cde0efee57..2f4d4329d83d 100644
> --- a/fs/proc/proc_sysctl.c
> +++ b/fs/proc/proc_sysctl.c
> @@ -48,10 +48,8 @@ struct ctl_table_header *register_sysctl_mount_point(const char *path)
> }
> EXPORT_SYMBOL(register_sysctl_mount_point);
>
> -#define sysctl_is_perm_empty_ctl_table(tptr) \
> - (tptr[0].type == SYSCTL_TABLE_TYPE_PERMANENTLY_EMPTY)
> #define sysctl_is_perm_empty_ctl_header(hptr) \
> - (sysctl_is_perm_empty_ctl_table(hptr->ctl_table))
> + (hptr->ctl_table[0].type == SYSCTL_TABLE_TYPE_PERMANENTLY_EMPTY)
> #define sysctl_set_perm_empty_ctl_header(hptr) \
> (hptr->ctl_table[0].type = SYSCTL_TABLE_TYPE_PERMANENTLY_EMPTY)
> #define sysctl_clear_perm_empty_ctl_header(hptr) \
> @@ -233,7 +231,7 @@ static int insert_header(struct ctl_dir *dir, struct ctl_table_header *header)
>
> /* Am I creating a permanently empty directory? */
> if (header->ctl_table_size > 0 &&
> - sysctl_is_perm_empty_ctl_table(header->ctl_table)) {
> + sysctl_is_perm_empty_ctl_header(header)) {
> if (!RB_EMPTY_ROOT(&dir->root))
> return -EINVAL;
> sysctl_set_perm_empty_ctl_header(dir_h);
> @@ -1204,7 +1202,7 @@ static bool get_links(struct ctl_dir *dir,
> struct ctl_table *entry, *link;
>
> if (header->ctl_table_size == 0 ||
> - sysctl_is_perm_empty_ctl_table(header->ctl_table))
> + sysctl_is_perm_empty_ctl_header(header))
> return true;
>
> /* Are there links available for every entry in table? */
>
> --
> 2.43.2
>
--
Joel Granados
On Thu, Feb 22, 2024 at 08:07:35AM +0100, Thomas Wei?schuh wrote:
> Praparation series to enable constification of struct ctl_table further
> down the line.
> No functional changes are intended.
Took me some time but I finally got around to it. I just had comments
regarding the commit messages. I'll add this to sysctl testing to get
the ball rolling; I'll add it to sysctl-next after you modify the commit
messages.
Thx.
>
> These changes have been split out and reworked from my original
> const sysctl patchset [0].
> I'm resubmitting the patchset in smaller chunks for easier review.
> Each split-out series is meant to be useful on its own.
>
> Changes since the original series:
> * Explicit initializartion of header->type in init_header()
> * Some additional cleanups
>
> [0] https://lore.kernel.org/lkml/[email protected]/
>
> ---
> Thomas Wei?schuh (4):
> sysctl: drop sysctl_is_perm_empty_ctl_table
> sysctl: move sysctl type to ctl_table_header
> sysctl: drop now unnecessary out-of-bounds check
> sysctl: remove unnecessary sentinel element
>
> fs/proc/proc_sysctl.c | 19 ++++++++-----------
> include/linux/sysctl.h | 22 +++++++++++-----------
> 2 files changed, 19 insertions(+), 22 deletions(-)
> ---
> base-commit: b401b621758e46812da61fa58a67c3fd8d91de0d
> change-id: 20231216-sysctl-empty-dir-71d7631f7bfe
>
> Best regards,
> --
> Thomas Wei?schuh <[email protected]>
>
--
Joel Granados