Re: [PATCH 1/4] Use in-place path separator "/" for Linux specific code

Re: [PATCH 1/4] Use in-place path separator "/" for Linux specific code

[Jens Axboe]

On 07/29/2016 09:05 AM, Tomohiro Kusumi wrote:
> diskutil,cgroup,blktrace related code fully depend on Linux kernel,
> so sprintf variants can use in-place path separator "/" instead of
> FIO_OS_PATH_SEPARATOR.
>
> In fact these Linux specific files are mix of two types within the
> same file depending on who wrote them, where they were originally
> using "/".

Thanks, applied this and the other 3.

-- 
Jens Axboe

[PATCH 1/4] Use in-place path separator "/" for Linux specific code

[Tomohiro Kusumi]

diskutil,cgroup,blktrace related code fully depend on Linux kernel,
so sprintf variants can use in-place path separator "/" instead of
FIO_OS_PATH_SEPARATOR.

In fact these Linux specific files are mix of two types within the
same file depending on who wrote them, where they were originally
using "/".

Signed-off-by: Tomohiro Kusumi <kusumi.tomohiro@gmail.com>
---
 cgroup.c                 |    6 +++---
 diskutil.c               |    6 +++---
 oslib/linux-dev-lookup.c |    2 +-
 3 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/cgroup.c b/cgroup.c
index 34b61de..a297e2a 100644
--- a/cgroup.c
+++ b/cgroup.c
@@ -102,9 +102,9 @@ static char *get_cgroup_root(struct thread_data *td, char *mnt)
 	char *str = malloc(64);
 
 	if (td->o.cgroup)
-		sprintf(str, "%s%s%s", mnt, FIO_OS_PATH_SEPARATOR, td->o.cgroup);
+		sprintf(str, "%s/%s", mnt, td->o.cgroup);
 	else
-		sprintf(str, "%s%s%s", mnt, FIO_OS_PATH_SEPARATOR, td->o.name);
+		sprintf(str, "%s/%s", mnt, td->o.name);
 
 	return str;
 }
@@ -116,7 +116,7 @@ static int write_int_to_file(struct thread_data *td, const char *path,
 	char tmp[256];
 	FILE *f;
 
-	sprintf(tmp, "%s%s%s", path, FIO_OS_PATH_SEPARATOR, filename);
+	sprintf(tmp, "%s/%s", path, filename);
 	f = fopen(tmp, "w");
 	if (!f) {
 		td_verror(td, errno, onerr);
diff --git a/diskutil.c b/diskutil.c
index 8031d5d..294d2d3 100644
--- a/diskutil.c
+++ b/diskutil.c
@@ -239,7 +239,7 @@ static void find_add_disk_slaves(struct thread_data *td, char *path,
 		    !strcmp(dirent->d_name, ".."))
 			continue;
 
-		sprintf(temppath, "%s%s%s", slavesdir, FIO_OS_PATH_SEPARATOR, dirent->d_name);
+		sprintf(temppath, "%s/%s", slavesdir, dirent->d_name);
 		/* Can we always assume that the slaves device entries
 		 * are links to the real directories for the slave
 		 * devices?
@@ -266,7 +266,7 @@ static void find_add_disk_slaves(struct thread_data *td, char *path,
 		if (slavedu)
 			continue;
 
-		sprintf(temppath, "%s%s%s", slavesdir, FIO_OS_PATH_SEPARATOR, slavepath);
+		sprintf(temppath, "%s/%s", slavesdir, slavepath);
 		__init_per_file_disk_util(td, majdev, mindev, temppath);
 		slavedu = disk_util_exists(majdev, mindev);
 
@@ -370,7 +370,7 @@ static int find_block_dir(int majdev, int mindev, char *path, int link_ok)
 		if (!strcmp(dir->d_name, ".") || !strcmp(dir->d_name, ".."))
 			continue;
 
-		sprintf(full_path, "%s%s%s", path, FIO_OS_PATH_SEPARATOR, dir->d_name);
+		sprintf(full_path, "%s/%s", path, dir->d_name);
 
 		if (!strcmp(dir->d_name, "dev")) {
 			if (!check_dev_match(majdev, mindev, full_path)) {
diff --git a/oslib/linux-dev-lookup.c b/oslib/linux-dev-lookup.c
index 4d5f356..3a415dd 100644
--- a/oslib/linux-dev-lookup.c
+++ b/oslib/linux-dev-lookup.c
@@ -25,7 +25,7 @@ int blktrace_lookup_device(const char *redirect, char *path, unsigned int maj,
 		if (!strcmp(dir->d_name, ".") || !strcmp(dir->d_name, ".."))
 			continue;
 
-		sprintf(full_path, "%s%s%s", path, FIO_OS_PATH_SEPARATOR, dir->d_name);
+		sprintf(full_path, "%s/%s", path, dir->d_name);
 		if (lstat(full_path, &st) == -1) {
 			perror("lstat");
 			break;
-- 
1.7.1

[PATCH 2/4] Make switch_ioscheduler() return 0 if FIO_HAVE_IOSCHED_SWITCH is undefined

[Tomohiro Kusumi]

Defining FIO_HAVE_IOSCHED_SWITCH currently equals saying it's Linux,
as switch_ioscheduler() only works on Linux kernel with "scheduler"
sysfs entry (though read/write to sysfs obviously compiles on others).

This commit makes the function return 0 if FIO_HAVE_IOSCHED_SWITCH
is undefined (i.e. if not Linux). This is essentially the same as
{diskutil,cgroup,blktrace}.c being compiled only on Linux.

Signed-off-by: Tomohiro Kusumi <kusumi.tomohiro@gmail.com>
---
 backend.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/backend.c b/backend.c
index ad2d7da..2f290d2 100644
--- a/backend.c
+++ b/backend.c
@@ -1261,6 +1261,7 @@ static int init_io_u(struct thread_data *td)
 
 static int switch_ioscheduler(struct thread_data *td)
 {
+#ifdef FIO_HAVE_IOSCHED_SWITCH
 	char tmp[256], tmp2[128];
 	FILE *f;
 	int ret;
@@ -1319,6 +1320,9 @@ static int switch_ioscheduler(struct thread_data *td)
 
 	fclose(f);
 	return 0;
+#else
+	return 0;
+#endif
 }
 
 static bool keep_running(struct thread_data *td)
-- 
1.7.1

[PATCH 3/4] Null terminate before (or after) strncpy(3)

[Tomohiro Kusumi]

These three strncpy() calls copy at most sizeof(buffer)-1 bytes,
but buffer isn't explicitly 0 cleared, so 0 terminate the last byte.

Signed-off-by: Tomohiro Kusumi <kusumi.tomohiro@gmail.com>
---
 diskutil.c  |    2 ++
 ioengines.c |    1 +
 2 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/diskutil.c b/diskutil.c
index 294d2d3..a1077d4 100644
--- a/diskutil.c
+++ b/diskutil.c
@@ -179,6 +179,7 @@ static int get_device_numbers(char *file_name, int *maj, int *min)
 		/*
 		 * must be a file, open "." in that path
 		 */
+		tempname[PATH_MAX - 1] = '\0';
 		strncpy(tempname, file_name, PATH_MAX - 1);
 		p = dirname(tempname);
 		if (stat(p, &st)) {
@@ -426,6 +427,7 @@ static struct disk_util *__init_per_file_disk_util(struct thread_data *td,
 			log_err("unknown sysfs layout\n");
 			return NULL;
 		}
+		tmp[PATH_MAX - 1] = '\0';
 		strncpy(tmp, p, PATH_MAX - 1);
 		sprintf(path, "%s", tmp);
 	}
diff --git a/ioengines.c b/ioengines.c
index 918b50a..f7b5ed6 100644
--- a/ioengines.c
+++ b/ioengines.c
@@ -130,6 +130,7 @@ struct ioengine_ops *load_ioengine(struct thread_data *td, const char *name)
 
 	dprint(FD_IO, "load ioengine %s\n", name);
 
+	engine[sizeof(engine) - 1] = '\0';
 	strncpy(engine, name, sizeof(engine) - 1);
 
 	/*
-- 
1.7.1

[PATCH 4/4] Use larger local buffer for I/O engine name

[Tomohiro Kusumi]

16 bytes may not be large enough in the future if a new I/O engine
with a name longer than strlen(name)=15 is added.

The longest one right now seems to be "fusion-aw-sync" which is 14(+1).

Signed-off-by: Tomohiro Kusumi <kusumi.tomohiro@gmail.com>
---
 ioengines.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/ioengines.c b/ioengines.c
index f7b5ed6..4129ac2 100644
--- a/ioengines.c
+++ b/ioengines.c
@@ -126,7 +126,7 @@ static struct ioengine_ops *dlopen_ioengine(struct thread_data *td,
 struct ioengine_ops *load_ioengine(struct thread_data *td, const char *name)
 {
 	struct ioengine_ops *ops;
-	char engine[16];
+	char engine[64];
 
 	dprint(FD_IO, "load ioengine %s\n", name);
 
-- 
1.7.1

Re: [PATCH 1/4] Use in-place path separator "/" for Linux specific code

[Tomohiro Kusumi]

Thank you !

2016-07-30 0:02 GMT+09:00 Jens Axboe <axboe@kernel.dk>:
> On 07/29/2016 09:05 AM, Tomohiro Kusumi wrote:
>>
>> diskutil,cgroup,blktrace related code fully depend on Linux kernel,
>> so sprintf variants can use in-place path separator "/" instead of
>> FIO_OS_PATH_SEPARATOR.
>>
>> In fact these Linux specific files are mix of two types within the
>> same file depending on who wrote them, where they were originally
>> using "/".
>
>
> Thanks, applied this and the other 3.
>
> --
> Jens Axboe
>