[Tarantool-patches] [PATCH 0/6] xlog: cleanup patches

[Tarantool-patches] [PATCH 0/6] xlog: cleanup patches

[Cyrill Gorcunov]

While been working on the feature #4743 I've noticed that we've
a pure mess in PATH_MAX usage inside xlog code. Lets clean it
a bit while we're here. Basically the idea is to not use PATH_MAX+1
constant, the PATH_MAX already implies to include end of string.

branch gorcunov/gh-4743-path-max-cleanup

Cyrill Gorcunov (6):
  box: drop inline from box_cfg_xc
  xlog: xlog_cursor -- eliminate redundant pad in the structure
  xlog: xdir -- use PATH_MAX for dirname
  xlog: use PATH_MAX for filename
  xlog: xlog_cursor -- use sizeof with snprintf for safety
  xlog: xdir_format_filename -- use PATH_MAX

 src/box/box.cc |  2 +-
 src/box/xlog.c | 14 ++++++++------
 src/box/xlog.h | 12 ++++++------
 3 files changed, 15 insertions(+), 13 deletions(-)

-- 
2.26.2

[Tarantool-patches] [PATCH 1/6] box: drop inline from box_cfg_xc

[Cyrill Gorcunov]

There is serious "inline disease" in the code:
it spread left and right without a serious reason.

The box_cfg_xc function is a pretty big one and
doesn't require being inlined anyhow.

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
 src/box/box.cc | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/box/box.cc b/src/box/box.cc
index 96557651b..7c8b70b27 100644
--- a/src/box/box.cc
+++ b/src/box/box.cc
@@ -2373,7 +2373,7 @@ box_is_configured(void)
 	return is_box_configured;
 }
 
-static inline void
+static void
 box_cfg_xc(void)
 {
 	/* Join the cord interconnect as "tx" endpoint. */
-- 
2.26.2

[Tarantool-patches] [PATCH 2/6] xlog: xlog_cursor -- eliminate redundant pad in the structure

[Cyrill Gorcunov]

This makes structure less in size and eliminates useless
padding (both enum and fd are integers 4 bytes long).

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
 src/box/xlog.h | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/src/box/xlog.h b/src/box/xlog.h
index a48b05fc4..47df3a549 100644
--- a/src/box/xlog.h
+++ b/src/box/xlog.h
@@ -649,13 +649,13 @@ enum xlog_cursor_state {
 /**
  * Xlog cursor, read rows from xlog
  */
-struct xlog_cursor
-{
+struct xlog_cursor {
+	/** cursor current state */
 	enum xlog_cursor_state state;
-	/** xlog meta info */
-	struct xlog_meta meta;
 	/** file descriptor or -1 for in memory */
 	int fd;
+	/** xlog meta info */
+	struct xlog_meta meta;
 	/** associated file name */
 	char name[PATH_MAX];
 	/** file read buffer */
-- 
2.26.2

[Tarantool-patches] [PATCH 3/6] xlog: xdir -- use PATH_MAX for dirname

[Cyrill Gorcunov]

The PATH_MAX is the longest path including end
of string, no need for +1 byte.

Same time use sizeof(dirname) to not bound how
exactly dirname is declared.

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
 src/box/xlog.c | 2 +-
 src/box/xlog.h | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/box/xlog.c b/src/box/xlog.c
index 8254cce20..8b2818326 100644
--- a/src/box/xlog.c
+++ b/src/box/xlog.c
@@ -335,7 +335,7 @@ xdir_create(struct xdir *dir, const char *dirname, enum xdir_type type,
 	/* Default mode. */
 	dir->mode = 0660;
 	dir->instance_uuid = instance_uuid;
-	snprintf(dir->dirname, PATH_MAX, "%s", dirname);
+	snprintf(dir->dirname, sizeof(dir->dirname), "%s", dirname);
 	dir->open_wflags = 0;
 	switch (type) {
 	case SNAP:
diff --git a/src/box/xlog.h b/src/box/xlog.h
index 47df3a549..329d78c77 100644
--- a/src/box/xlog.h
+++ b/src/box/xlog.h
@@ -162,7 +162,7 @@ struct xdir {
 	/**
 	 * Directory path.
 	 */
-	char dirname[PATH_MAX+1];
+	char dirname[PATH_MAX];
 	/** Snapshots or xlogs */
 	enum xdir_type type;
 };
-- 
2.26.2

[Tarantool-patches] [PATCH 4/6] xlog: use PATH_MAX for filename

[Cyrill Gorcunov]

Similar to dirname there is no need for +1 byte.
Same time make sure xlog_open never end up without
trailing zero.

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
 src/box/xlog.c | 6 ++++--
 src/box/xlog.h | 2 +-
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/src/box/xlog.c b/src/box/xlog.c
index 8b2818326..1685a4cf7 100644
--- a/src/box/xlog.c
+++ b/src/box/xlog.c
@@ -822,7 +822,7 @@ xlog_create(struct xlog *xlog, const char *name, int flags,
 
 	xlog->meta = *meta;
 	xlog->is_inprogress = true;
-	snprintf(xlog->filename, PATH_MAX, "%s%s", name, inprogress_suffix);
+	snprintf(xlog->filename, sizeof(xlog->filename), "%s%s", name, inprogress_suffix);
 
 	flags |= O_RDWR | O_CREAT | O_EXCL;
 
@@ -881,7 +881,9 @@ xlog_open(struct xlog *xlog, const char *name, const struct xlog_opts *opts)
 	if (xlog_init(xlog, opts) != 0)
 		goto err;
 
-	strncpy(xlog->filename, name, PATH_MAX);
+	strncpy(xlog->filename, name, sizeof(xlog->filename));
+	xlog->filename[sizeof(xlog->filename)-1] = '\0';
+
 	xlog->fd = open(xlog->filename, O_RDWR);
 	if (xlog->fd < 0) {
 		say_syserror("open, [%s]", name);
diff --git a/src/box/xlog.h b/src/box/xlog.h
index 329d78c77..9ffce598b 100644
--- a/src/box/xlog.h
+++ b/src/box/xlog.h
@@ -352,7 +352,7 @@ struct xlog {
 	 */
 	int64_t tx_rows;
 	/** Log file name. */
-	char filename[PATH_MAX + 1];
+	char filename[PATH_MAX];
 	/** Whether this file has .inprogress suffix. */
 	bool is_inprogress;
 	/*
-- 
2.26.2

Re: [Tarantool-patches] [PATCH 4/6] xlog: use PATH_MAX for filename

[Vladislav Shpilevoy]

Hi! Thanks for the patch!

> diff --git a/src/box/xlog.c b/src/box/xlog.c
> index 8b2818326..1685a4cf7 100644
> --- a/src/box/xlog.c
> +++ b/src/box/xlog.c
> @@ -881,7 +881,9 @@ xlog_open(struct xlog *xlog, const char *name, const struct xlog_opts *opts)
>  	if (xlog_init(xlog, opts) != 0)
>  		goto err;
>  
> -	strncpy(xlog->filename, name, PATH_MAX);
> +	strncpy(xlog->filename, name, sizeof(xlog->filename));
> +	xlog->filename[sizeof(xlog->filename)-1] = '\0';

Please, use whitespaces before and after binary operations.

Re: [Tarantool-patches] [PATCH 4/6] xlog: use PATH_MAX for filename

[Cyrill Gorcunov]

On Fri, Jun 19, 2020 at 01:14:22AM +0200, Vladislav Shpilevoy wrote:
> > --- a/src/box/xlog.c
> > +++ b/src/box/xlog.c
> > @@ -881,7 +881,9 @@ xlog_open(struct xlog *xlog, const char *name, const struct xlog_opts *opts)
> >  	if (xlog_init(xlog, opts) != 0)
> >  		goto err;
> >  
> > -	strncpy(xlog->filename, name, PATH_MAX);
> > +	strncpy(xlog->filename, name, sizeof(xlog->filename));
> > +	xlog->filename[sizeof(xlog->filename)-1] = '\0';
> 
> Please, use whitespaces before and after binary operations.

I could argue that this -1 is not a regular binop taking account
the context of the operation but sure, updated and pushed out.
Thanks!

[Tarantool-patches] [PATCH 5/6] xlog: xlog_cursor -- use sizeof with snprintf for safety

[Cyrill Gorcunov]

This is more consistent than relying that array size
will remain PATH_MAX forever.

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
 src/box/xlog.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/box/xlog.c b/src/box/xlog.c
index 1685a4cf7..94f7838bf 100644
--- a/src/box/xlog.c
+++ b/src/box/xlog.c
@@ -1970,7 +1970,7 @@ xlog_cursor_openfd(struct xlog_cursor *i, int fd, const char *name)
 		diag_set(XlogError, "Unexpected end of file, run with 'force_recovery = true'");
 		goto error;
 	}
-	snprintf(i->name, PATH_MAX, "%s", name);
+	snprintf(i->name, sizeof(i->name), "%s", name);
 	i->zdctx = ZSTD_createDStream();
 	if (i->zdctx == NULL) {
 		diag_set(ClientError, ER_DECOMPRESSION,
@@ -2027,7 +2027,7 @@ xlog_cursor_openmem(struct xlog_cursor *i, const char *data, size_t size,
 		diag_set(XlogError, "Unexpected end of file");
 		goto error;
 	}
-	snprintf(i->name, PATH_MAX, "%s", name);
+	snprintf(i->name, sizeof(i->name), "%s", name);
 	i->zdctx = ZSTD_createDStream();
 	if (i->zdctx == NULL) {
 		diag_set(ClientError, ER_DECOMPRESSION,
-- 
2.26.2

[Tarantool-patches] [PATCH 6/6] xlog: xdir_format_filename -- use PATH_MAX

[Cyrill Gorcunov]

No need for +1 byte here, PATH_MAX already implies
end of string.

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
 src/box/xlog.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/box/xlog.c b/src/box/xlog.c
index 94f7838bf..bf68c6925 100644
--- a/src/box/xlog.c
+++ b/src/box/xlog.c
@@ -650,7 +650,7 @@ const char *
 xdir_format_filename(struct xdir *dir, int64_t signature,
 		enum log_suffix suffix)
 {
-	return tt_snprintf(PATH_MAX + 1, "%s/%020lld%s%s",
+	return tt_snprintf(PATH_MAX, "%s/%020lld%s%s",
 			   dir->dirname, (long long) signature,
 			   dir->filename_ext, suffix == INPROGRESS ?
 					      inprogress_suffix : "");
-- 
2.26.2

Re: [Tarantool-patches] [PATCH 0/6] xlog: cleanup patches

[Vladislav Shpilevoy]

LGTM.

Re: [Tarantool-patches] [PATCH 0/6] xlog: cleanup patches

[Kirill Yukhin]

Hello,

On 18 июн 01:03, Cyrill Gorcunov wrote:
> While been working on the feature #4743 I've noticed that we've
> a pure mess in PATH_MAX usage inside xlog code. Lets clean it
> a bit while we're here. Basically the idea is to not use PATH_MAX+1
> constant, the PATH_MAX already implies to include end of string.
> 
> branch gorcunov/gh-4743-path-max-cleanup

I've checked your patchset into master.

--
Regards, Kirill Yukhin