2021-02-18 01:01:14

by Stotland, Inga

[permalink] [raw]
Subject: [PATCH BlueZ v2] mesh: Combine common functions for NetKeys and AppKeys

No change in functionality, code tightening.
---
mesh/keyring.c | 117 +++++++++++++++++++++----------------------------
1 file changed, 49 insertions(+), 68 deletions(-)

diff --git a/mesh/keyring.c b/mesh/keyring.c
index 0b74ee914..cb04437ed 100644
--- a/mesh/keyring.c
+++ b/mesh/keyring.c
@@ -33,31 +33,45 @@ const char *dev_key_dir = "/dev_keys";
const char *app_key_dir = "/app_keys";
const char *net_key_dir = "/net_keys";

-bool keyring_put_net_key(struct mesh_node *node, uint16_t net_idx,
- struct keyring_net_key *key)
+static int open_key_file(struct mesh_node *node, const char *key_dir,
+ uint16_t idx, int flags)
{
const char *node_path;
- char key_file[PATH_MAX];
- bool result = false;
- int fd;
+ char fname[PATH_MAX];

- if (!node || !key)
- return false;
+ if (!node)
+ return -1;

node_path = node_get_storage_dir(node);

- if (strlen(node_path) + strlen(net_key_dir) + 1 + 3 >= PATH_MAX)
- return false;
+ if (strlen(node_path) + strlen(key_dir) + 1 + 3 >= PATH_MAX)
+ return -1;
+
+ if (flags & O_CREAT) {
+ snprintf(fname, PATH_MAX, "%s%s", node_path, key_dir);
+ mkdir(fname, 0755);
+ }

- snprintf(key_file, PATH_MAX, "%s%s", node_path, net_key_dir);
+ snprintf(fname, PATH_MAX, "%s%s/%3.3x", node_path, key_dir, idx);

- mkdir(key_file, 0755);
+ if (flags & O_CREAT)
+ return open(fname, flags, S_IRUSR | S_IWUSR);
+ else
+ return open(fname, flags);
+}

- snprintf(key_file, PATH_MAX, "%s%s/%3.3x", node_path, net_key_dir,
- net_idx);
- l_debug("Put Net Key %s", key_file);
+bool keyring_put_net_key(struct mesh_node *node, uint16_t net_idx,
+ struct keyring_net_key *key)
+{
+ bool result = false;
+ int fd;
+
+ if (!key)
+ return false;
+
+ fd = open_key_file(node, net_key_dir, net_idx,
+ O_WRONLY | O_CREAT | O_TRUNC);

- fd = open(key_file, O_WRONLY | O_CREAT | O_TRUNC, S_IRUSR | S_IWUSR);
if (fd < 0)
return false;

@@ -72,28 +86,14 @@ bool keyring_put_net_key(struct mesh_node *node, uint16_t net_idx,
bool keyring_put_app_key(struct mesh_node *node, uint16_t app_idx,
uint16_t net_idx, struct keyring_app_key *key)
{
- const char *node_path;
- char key_file[PATH_MAX];
bool result = false;
int fd;

- if (!node || !key)
- return false;
-
- node_path = node_get_storage_dir(node);
-
- if (strlen(node_path) + strlen(app_key_dir) + 1 + 3 >= PATH_MAX)
+ if (!key)
return false;

- snprintf(key_file, PATH_MAX, "%s%s", node_path, app_key_dir);
-
- mkdir(key_file, 0755);
+ fd = open_key_file(node, app_key_dir, app_idx, O_RDWR);

- snprintf(key_file, PATH_MAX, "%s%s/%3.3x", node_path, app_key_dir,
- app_idx);
- l_debug("Put App Key %s", key_file);
-
- fd = open(key_file, O_RDWR);
if (fd >= 0) {
struct keyring_app_key old_key;

@@ -105,12 +105,12 @@ bool keyring_put_app_key(struct mesh_node *node, uint16_t app_idx,
}

lseek(fd, 0, SEEK_SET);
- } else {
- fd = open(key_file, O_WRONLY | O_CREAT | O_TRUNC,
- S_IRUSR | S_IWUSR);
- if (fd < 0)
- return false;
- }
+ } else
+ fd = open_key_file(node, app_key_dir, app_idx,
+ O_WRONLY | O_CREAT | O_TRUNC);
+
+ if (fd < 0)
+ return false;

if (write(fd, key, sizeof(*key)) == sizeof(*key))
result = true;
@@ -226,24 +226,19 @@ bool keyring_put_remote_dev_key(struct mesh_node *node, uint16_t unicast,
return result;
}

-bool keyring_get_net_key(struct mesh_node *node, uint16_t net_idx,
- struct keyring_net_key *key)
+static bool get_key(struct mesh_node *node, const char *key_dir,
+ uint16_t key_idx, void *key, ssize_t sz)
{
- const char *node_path;
- char key_file[PATH_MAX];
bool result = false;
int fd;

- if (!node || !key)
+ if (!key)
return false;

- node_path = node_get_storage_dir(node);
- snprintf(key_file, PATH_MAX, "%s%s/%3.3x", node_path, net_key_dir,
- net_idx);
+ fd = open_key_file(node, key_dir, key_idx, O_RDONLY);

- fd = open(key_file, O_RDONLY);
if (fd >= 0) {
- if (read(fd, key, sizeof(*key)) == sizeof(*key))
+ if (read(fd, key, sz) == sz)
result = true;

close(fd);
@@ -252,30 +247,16 @@ bool keyring_get_net_key(struct mesh_node *node, uint16_t net_idx,
return result;
}

+bool keyring_get_net_key(struct mesh_node *node, uint16_t net_idx,
+ struct keyring_net_key *key)
+{
+ return get_key(node, net_key_dir, net_idx, key, sizeof(*key));
+}
+
bool keyring_get_app_key(struct mesh_node *node, uint16_t app_idx,
struct keyring_app_key *key)
{
- const char *node_path;
- char key_file[PATH_MAX];
- bool result = false;
- int fd;
-
- if (!node || !key)
- return false;
-
- node_path = node_get_storage_dir(node);
- snprintf(key_file, PATH_MAX, "%s%s/%3.3x", node_path, app_key_dir,
- app_idx);
-
- fd = open(key_file, O_RDONLY);
- if (fd >= 0) {
- if (read(fd, key, sizeof(*key)) == sizeof(*key))
- result = true;
-
- close(fd);
- }
-
- return result;
+ return get_key(node, app_key_dir, app_idx, key, sizeof(*key));
}

bool keyring_get_remote_dev_key(struct mesh_node *node, uint16_t unicast,
--
2.26.2


2021-02-18 01:29:48

by bluez.test.bot

[permalink] [raw]
Subject: RE: [BlueZ,v2] mesh: Combine common functions for NetKeys and AppKeys

This is automated email and please do not reply to this email!

Dear submitter,

Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=434773

---Test result---

##############################
Test: CheckPatch - FAIL
Output:
mesh: Combine common functions for NetKeys and AppKeys
WARNING:SYMBOLIC_PERMS: Symbolic permissions 'S_IRUSR | S_IWUSR' are not preferred. Consider using octal permissions '0600'.
#49: FILE: mesh/keyring.c:58:
+ return open(fname, flags, S_IRUSR | S_IWUSR);

- total: 0 errors, 1 warnings, 174 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
mechanically convert to the typical style using --fix or --fix-inplace.

"[PATCH] mesh: Combine common functions for NetKeys and AppKeys" has style problems, please review.

NOTE: Ignored message types: COMMIT_MESSAGE COMPLEX_MACRO CONST_STRUCT FILE_PATH_CHANGES MISSING_SIGN_OFF PREFER_PACKED SPLIT_STRING SSCANF_TO_KSTRTO

NOTE: If any of the errors are false positives, please report
them to the maintainer, see CHECKPATCH in MAINTAINERS.


##############################
Test: CheckGitLint - PASS

##############################
Test: CheckBuild - PASS

##############################
Test: MakeCheck - PASS



---
Regards,
Linux Bluetooth

2021-02-18 19:14:35

by Gix, Brian

[permalink] [raw]
Subject: Re: [PATCH BlueZ v2] mesh: Combine common functions for NetKeys and AppKeys

Applied with additional tweek of changing file modes from symbolic names to 0600 to conform to new tests in
checkpatch and kernel standards.

On Wed, 2021-02-17 at 16:39 -0800, Inga Stotland wrote:
> No change in functionality, code tightening.
> ---
> mesh/keyring.c | 117 +++++++++++++++++++++----------------------------
> 1 file changed, 49 insertions(+), 68 deletions(-)
>
> diff --git a/mesh/keyring.c b/mesh/keyring.c
> index 0b74ee914..cb04437ed 100644
> --- a/mesh/keyring.c
> +++ b/mesh/keyring.c
> @@ -33,31 +33,45 @@ const char *dev_key_dir = "/dev_keys";
> const char *app_key_dir = "/app_keys";
> const char *net_key_dir = "/net_keys";
>
> -bool keyring_put_net_key(struct mesh_node *node, uint16_t net_idx,
> - struct keyring_net_key *key)
> +static int open_key_file(struct mesh_node *node, const char *key_dir,
> + uint16_t idx, int flags)
> {
> const char *node_path;
> - char key_file[PATH_MAX];
> - bool result = false;
> - int fd;
> + char fname[PATH_MAX];
>
> - if (!node || !key)
> - return false;
> + if (!node)
> + return -1;
>
> node_path = node_get_storage_dir(node);
>
> - if (strlen(node_path) + strlen(net_key_dir) + 1 + 3 >= PATH_MAX)
> - return false;
> + if (strlen(node_path) + strlen(key_dir) + 1 + 3 >= PATH_MAX)
> + return -1;
> +
> + if (flags & O_CREAT) {
> + snprintf(fname, PATH_MAX, "%s%s", node_path, key_dir);
> + mkdir(fname, 0755);
> + }
>
> - snprintf(key_file, PATH_MAX, "%s%s", node_path, net_key_dir);
> + snprintf(fname, PATH_MAX, "%s%s/%3.3x", node_path, key_dir, idx);
>
> - mkdir(key_file, 0755);
> + if (flags & O_CREAT)
> + return open(fname, flags, S_IRUSR | S_IWUSR);
> + else
> + return open(fname, flags);
> +}
>
> - snprintf(key_file, PATH_MAX, "%s%s/%3.3x", node_path, net_key_dir,
> - net_idx);
> - l_debug("Put Net Key %s", key_file);
> +bool keyring_put_net_key(struct mesh_node *node, uint16_t net_idx,
> + struct keyring_net_key *key)
> +{
> + bool result = false;
> + int fd;
> +
> + if (!key)
> + return false;
> +
> + fd = open_key_file(node, net_key_dir, net_idx,
> + O_WRONLY | O_CREAT | O_TRUNC);
>
> - fd = open(key_file, O_WRONLY | O_CREAT | O_TRUNC, S_IRUSR | S_IWUSR);
> if (fd < 0)
> return false;
>
> @@ -72,28 +86,14 @@ bool keyring_put_net_key(struct mesh_node *node, uint16_t net_idx,
> bool keyring_put_app_key(struct mesh_node *node, uint16_t app_idx,
> uint16_t net_idx, struct keyring_app_key *key)
> {
> - const char *node_path;
> - char key_file[PATH_MAX];
> bool result = false;
> int fd;
>
> - if (!node || !key)
> - return false;
> -
> - node_path = node_get_storage_dir(node);
> -
> - if (strlen(node_path) + strlen(app_key_dir) + 1 + 3 >= PATH_MAX)
> + if (!key)
> return false;
>
> - snprintf(key_file, PATH_MAX, "%s%s", node_path, app_key_dir);
> -
> - mkdir(key_file, 0755);
> + fd = open_key_file(node, app_key_dir, app_idx, O_RDWR);
>
> - snprintf(key_file, PATH_MAX, "%s%s/%3.3x", node_path, app_key_dir,
> - app_idx);
> - l_debug("Put App Key %s", key_file);
> -
> - fd = open(key_file, O_RDWR);
> if (fd >= 0) {
> struct keyring_app_key old_key;
>
> @@ -105,12 +105,12 @@ bool keyring_put_app_key(struct mesh_node *node, uint16_t app_idx,
> }
>
> lseek(fd, 0, SEEK_SET);
> - } else {
> - fd = open(key_file, O_WRONLY | O_CREAT | O_TRUNC,
> - S_IRUSR | S_IWUSR);
> - if (fd < 0)
> - return false;
> - }
> + } else
> + fd = open_key_file(node, app_key_dir, app_idx,
> + O_WRONLY | O_CREAT | O_TRUNC);
> +
> + if (fd < 0)
> + return false;
>
> if (write(fd, key, sizeof(*key)) == sizeof(*key))
> result = true;
> @@ -226,24 +226,19 @@ bool keyring_put_remote_dev_key(struct mesh_node *node, uint16_t unicast,
> return result;
> }
>
> -bool keyring_get_net_key(struct mesh_node *node, uint16_t net_idx,
> - struct keyring_net_key *key)
> +static bool get_key(struct mesh_node *node, const char *key_dir,
> + uint16_t key_idx, void *key, ssize_t sz)
> {
> - const char *node_path;
> - char key_file[PATH_MAX];
> bool result = false;
> int fd;
>
> - if (!node || !key)
> + if (!key)
> return false;
>
> - node_path = node_get_storage_dir(node);
> - snprintf(key_file, PATH_MAX, "%s%s/%3.3x", node_path, net_key_dir,
> - net_idx);
> + fd = open_key_file(node, key_dir, key_idx, O_RDONLY);
>
> - fd = open(key_file, O_RDONLY);
> if (fd >= 0) {
> - if (read(fd, key, sizeof(*key)) == sizeof(*key))
> + if (read(fd, key, sz) == sz)
> result = true;
>
> close(fd);
> @@ -252,30 +247,16 @@ bool keyring_get_net_key(struct mesh_node *node, uint16_t net_idx,
> return result;
> }
>
> +bool keyring_get_net_key(struct mesh_node *node, uint16_t net_idx,
> + struct keyring_net_key *key)
> +{
> + return get_key(node, net_key_dir, net_idx, key, sizeof(*key));
> +}
> +
> bool keyring_get_app_key(struct mesh_node *node, uint16_t app_idx,
> struct keyring_app_key *key)
> {
> - const char *node_path;
> - char key_file[PATH_MAX];
> - bool result = false;
> - int fd;
> -
> - if (!node || !key)
> - return false;
> -
> - node_path = node_get_storage_dir(node);
> - snprintf(key_file, PATH_MAX, "%s%s/%3.3x", node_path, app_key_dir,
> - app_idx);
> -
> - fd = open(key_file, O_RDONLY);
> - if (fd >= 0) {
> - if (read(fd, key, sizeof(*key)) == sizeof(*key))
> - result = true;
> -
> - close(fd);
> - }
> -
> - return result;
> + return get_key(node, app_key_dir, app_idx, key, sizeof(*key));
> }
>
> bool keyring_get_remote_dev_key(struct mesh_node *node, uint16_t unicast,