[PATCH 0/8] option/Makefile fix and void* fixups

[PATCH 0/8] option/Makefile fix and void* fixups

[kusumi.tomohiro]

From: Tomohiro Kusumi <tkusumi@tuxera.com>

3/8 to 8/8 are the continuation of void* fixups from below,
but this time in non-ioengine code.
This is it and no more of this void* patches...

http://www.spinics.net/lists/fio/msg05912.html
>   sg: don't use void* for pointer arithmetic (gcc)
>   splice: don't use void* for pointer arithmetic (gcc)
>   binject: don't use void* for pointer arithmetic (gcc)

Tomohiro Kusumi (8):
  ignore directory= if the given path is absolute
  Makefile: use fmt(1) rather than tr(1) on NetBSD/etc
  server: don't use void* for pointer arithmetic (gcc)
  io_u: don't use void* for pointer arithmetic (gcc)
  init: don't use void* for pointer arithmetic (gcc)
  client: don't use void* for pointer arithmetic (gcc)
  verify: don't use void* for pointer arithmetic (gcc)
  smalloc: don't use void* for pointer arithmetic (gcc)

 HOWTO            |  3 ++-
 Makefile         | 10 ++++++++++
 client.c         | 12 ++++++------
 filesetup.c      | 10 +++++++---
 init.c           |  2 +-
 io_u.c           |  2 +-
 server.c         |  5 +++--
 smalloc.c        |  2 +-
 t/verify-state.c |  3 ++-
 verify-state.h   |  2 +-
 verify.c         |  7 ++++---
 11 files changed, 38 insertions(+), 20 deletions(-)

-- 
2.9.4

[PATCH 1/8] ignore directory= if the given path is absolute

[kusumi.tomohiro]

From: Tomohiro Kusumi <tkusumi@tuxera.com>

When filename= is specified in absolute path format (starts with '/'),
being able to use directory= doesn't make much sense and also likely
to result in an unexpected error, thus it should be ignored if any.

(If combining directory= and absolute filename= is a feature rather
than unexpected, then it should be documented, since it works as long
as directory/filename doesn't contain invalid directory. For example,
directory=. and filename=/tmp/xxx work as ./tmp/xxx if ./tmp exists
as shown in below.)

Not sure about the proper way to handle this on Windows, so I had to
add WIN32 guard.

--
 # ./fio --version
 fio-2.21-26-gcf6b7
 # ls ./tmp
 ls: cannot access './tmp': No such file or directory
 # ./fio --name=xxxxx --ioengine=sync --rw=read --size=1m --directory=. --filename=/tmp/xxx > /dev/null; echo $?
 fio: pid=0, err=2/file:filesetup.c:99, func=open, error=No such file or directory
 1
 # mkdir ./tmp
 # ./fio --name=xxxxx --ioengine=sync --rw=read --size=1m --directory=. --filename=/tmp/xxx > /dev/null; echo $?
 0
 # ls ./tmp
 xxx
 # ls /tmp/xxx
 ls: cannot access '/tmp/xxx': No such file or directory

Signed-off-by: Tomohiro Kusumi <tkusumi@tuxera.com>
---
 HOWTO       |  3 ++-
 filesetup.c | 10 +++++++---
 2 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/HOWTO b/HOWTO
index 22c5a5b..ec1352b 100644
--- a/HOWTO
+++ b/HOWTO
@@ -727,7 +727,8 @@ Target file/device
 	long as they are using generated filenames. If specific `filename(s)` are
 	set fio will use the first listed directory, and thereby matching the
 	`filename` semantic which generates a file each clone if not specified, but
-	let all clones use the same if set.
+	let all clones use the same if set. If `filename(s)` are given as absolute
+	paths, this option is ignored (except for on Windows).
 
 	See the :option:`filename` option for escaping certain characters.
 
diff --git a/filesetup.c b/filesetup.c
index 13079e4..ae74ea1 100644
--- a/filesetup.c
+++ b/filesetup.c
@@ -1494,9 +1494,13 @@ int add_file(struct thread_data *td, const char *fname, int numjob, int inc)
 
 	dprint(FD_FILE, "add file %s\n", fname);
 
-	if (td->o.directory)
-		len = set_name_idx(file_name, PATH_MAX, td->o.directory, numjob,
-					td->o.unique_filename);
+	if (td->o.directory) {
+#ifndef WIN32
+		if (fname[0] != '/')
+#endif
+			len = set_name_idx(file_name, PATH_MAX, td->o.directory,
+						numjob, td->o.unique_filename);
+	}
 
 	sprintf(file_name + len, "%s", fname);
 
-- 
2.9.4

[PATCH 2/8] Makefile: use fmt(1) rather than tr(1) on NetBSD/etc

[kusumi.tomohiro]

From: Tomohiro Kusumi <tkusumi@tuxera.com>

e291cff1 (Use fmt -w WIDTH option instead of -WIDTH) changed the
command option to a common one to suppress an error, but it only
fixed the unknown option error without being functional on NetBSD.

This change is taken from NetBSD's pkgsrc. It may work against
other platforms that don't work with fmt(1), but only enabled for
NetBSD at the moment. FreeBSD/DragonFlyBSD/OpenBSD work with fmt(1).

(This actually works on Linux too, but the existing one should be
kept for Linux (and other platforms) provided it has been used on
various distros for years)

https://github.com/NetBSD/pkgsrc/blob/trunk/benchmarks/fio/patches/patch-Makefile

> Convert the fmt(1) command to a tr(1) one (the fmt(1) old syntax
> command is not supported on all Unix systems).

Signed-off-by: Tomohiro Kusumi <tkusumi@tuxera.com>
---
 Makefile | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/Makefile b/Makefile
index 64fa97a..bef930f 100644
--- a/Makefile
+++ b/Makefile
@@ -327,8 +327,13 @@ override CFLAGS += -DFIO_VERSION='"$(FIO_VERSION)"'
 	@$(CC) -MM $(CFLAGS) $(CPPFLAGS) $(SRCDIR)/$*.c > $*.d
 	@mv -f $*.d $*.d.tmp
 	@sed -e 's|.*:|$*.o:|' < $*.d.tmp > $*.d
+ifeq ($(CONFIG_TARGET_OS), NetBSD)
+	@sed -e 's/.*://' -e 's/\\$$//' < $*.d.tmp | tr -cs "[:graph:]" "\n" | \
+		sed -e 's/^ *//' -e '/^$$/ d' -e 's/$$/:/' >> $*.d
+else
 	@sed -e 's/.*://' -e 's/\\$$//' < $*.d.tmp | fmt -w 1 | \
 		sed -e 's/^ *//' -e 's/$$/:/' >> $*.d
+endif
 	@rm -f $*.d.tmp
 
 ifdef CONFIG_ARITHMETIC
@@ -366,8 +371,13 @@ init.o: init.c FIO-VERSION-FILE
 	@$(CC) -MM $(CFLAGS) $(CPPFLAGS) $(SRCDIR)/$*.c > $*.d
 	@mv -f $*.d $*.d.tmp
 	@sed -e 's|.*:|$*.o:|' < $*.d.tmp > $*.d
+ifeq ($(CONFIG_TARGET_OS), NetBSD)
+	@sed -e 's/.*://' -e 's/\\$$//' < $*.d.tmp | tr -cs "[:graph:]" "\n" | \
+		sed -e 's/^ *//' -e '/^$$/ d' -e 's/$$/:/' >> $*.d
+else
 	@sed -e 's/.*://' -e 's/\\$$//' < $*.d.tmp | fmt -w 1 | \
 		sed -e 's/^ *//' -e 's/$$/:/' >> $*.d
+endif
 	@rm -f $*.d.tmp
 
 gcompat.o: gcompat.c gcompat.h
-- 
2.9.4

[PATCH 3/8] server: don't use void* for pointer arithmetic (gcc)

[kusumi.tomohiro]

From: Tomohiro Kusumi <tkusumi@tuxera.com>

I think gcc extension should be avoided when it can be done by
just changing a pointer type.

Signed-off-by: Tomohiro Kusumi <tkusumi@tuxera.com>
---
 server.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/server.c b/server.c
index 8b36e38..2d17df6 100644
--- a/server.c
+++ b/server.c
@@ -252,9 +252,10 @@ static int fio_send_data(int sk, const void *p, unsigned int len)
 	return fio_sendv_data(sk, &iov, 1);
 }
 
-static int fio_recv_data(int sk, void *p, unsigned int len, bool wait)
+static int fio_recv_data(int sk, void *buf, unsigned int len, bool wait)
 {
 	int flags;
+	char *p = buf;
 
 	if (wait)
 		flags = MSG_WAITALL;
@@ -377,7 +378,7 @@ struct fio_net_cmd *fio_net_recv_cmd(int sk, bool wait)
 			break;
 
 		/* There's payload, get it */
-		pdu = (void *) cmdret->payload + pdu_offset;
+		pdu = (char *) cmdret->payload + pdu_offset;
 		ret = fio_recv_data(sk, pdu, cmd.pdu_len, wait);
 		if (ret)
 			break;
-- 
2.9.4

[PATCH 4/8] io_u: don't use void* for pointer arithmetic (gcc)

[kusumi.tomohiro]

From: Tomohiro Kusumi <tkusumi@tuxera.com>

I think gcc extension should be avoided when it can be done by
just changing a pointer type.

Signed-off-by: Tomohiro Kusumi <tkusumi@tuxera.com>
---
 io_u.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/io_u.c b/io_u.c
index 375413f..8d42d65 100644
--- a/io_u.c
+++ b/io_u.c
@@ -1602,7 +1602,7 @@ static void small_content_scramble(struct io_u *io_u)
 	unsigned int i, nr_blocks = io_u->buflen / 512;
 	uint64_t boffset;
 	unsigned int offset;
-	void *p, *end;
+	char *p, *end;
 
 	if (!nr_blocks)
 		return;
-- 
2.9.4

[PATCH 5/8] init: don't use void* for pointer arithmetic (gcc)

[kusumi.tomohiro]

From: Tomohiro Kusumi <tkusumi@tuxera.com>

I think gcc extension should be avoided when it can be done by
just changing a pointer type.

(threads is a pointer to struct thread_data)

Signed-off-by: Tomohiro Kusumi <tkusumi@tuxera.com>
---
 init.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/init.c b/init.c
index 2b7768a..6efa5c1 100644
--- a/init.c
+++ b/init.c
@@ -361,7 +361,7 @@ static int setup_thread_area(void)
 #endif
 
 	memset(threads, 0, max_jobs * sizeof(struct thread_data));
-	fio_debug_jobp = (void *) threads + max_jobs * sizeof(struct thread_data);
+	fio_debug_jobp = (unsigned int *)(threads + max_jobs);
 	*fio_debug_jobp = -1;
 
 	flow_init();
-- 
2.9.4

[PATCH 6/8] client: don't use void* for pointer arithmetic (gcc)

[kusumi.tomohiro]

From: Tomohiro Kusumi <tkusumi@tuxera.com>

I think gcc extension should be avoided when it can be done by
just changing a pointer type.

Signed-off-by: Tomohiro Kusumi <tkusumi@tuxera.com>
---
 client.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/client.c b/client.c
index 7a986aa..960c795 100644
--- a/client.c
+++ b/client.c
@@ -1452,7 +1452,7 @@ static struct cmd_iolog_pdu *convert_iolog_gz(struct fio_net_cmd *cmd,
 	z_stream stream;
 	uint32_t nr_samples;
 	size_t total;
-	void *p;
+	char *p;
 
 	stream.zalloc = Z_NULL;
 	stream.zfree = Z_NULL;
@@ -1478,10 +1478,10 @@ static struct cmd_iolog_pdu *convert_iolog_gz(struct fio_net_cmd *cmd,
 
 	memcpy(ret, pdu, sizeof(*pdu));
 
-	p = (void *) ret + sizeof(*pdu);
+	p = (char *) ret + sizeof(*pdu);
 
 	stream.avail_in = cmd->pdu_len - sizeof(*pdu);
-	stream.next_in = (void *) pdu + sizeof(*pdu);
+	stream.next_in = (void *)((char *) pdu + sizeof(*pdu));
 	while (stream.avail_in) {
 		unsigned int this_chunk = 65536;
 		unsigned int this_len;
@@ -1491,7 +1491,7 @@ static struct cmd_iolog_pdu *convert_iolog_gz(struct fio_net_cmd *cmd,
 			this_chunk = total;
 
 		stream.avail_out = this_chunk;
-		stream.next_out = p;
+		stream.next_out = (void *)p;
 		err = inflate(&stream, Z_NO_FLUSH);
 		/* may be Z_OK, or Z_STREAM_END */
 		if (err < 0) {
@@ -1566,7 +1566,7 @@ static struct cmd_iolog_pdu *convert_iolog(struct fio_net_cmd *cmd,
 
 		s = __get_sample(samples, ret->log_offset, i);
 		if (ret->log_type == IO_LOG_TYPE_HIST)
-			s = (struct io_sample *)((void *)s + sizeof(struct io_u_plat_entry) * i);
+			s = (struct io_sample *)((char *)s + sizeof(struct io_u_plat_entry) * i);
 
 		s->time		= le64_to_cpu(s->time);
 		s->data.val	= le64_to_cpu(s->data.val);
@@ -1580,7 +1580,7 @@ static struct cmd_iolog_pdu *convert_iolog(struct fio_net_cmd *cmd,
 		}
 
 		if (ret->log_type == IO_LOG_TYPE_HIST) {
-			s->data.plat_entry = (struct io_u_plat_entry *)(((void *)s) + sizeof(*s));
+			s->data.plat_entry = (struct io_u_plat_entry *)(((char *)s) + sizeof(*s));
 			s->data.plat_entry->list.next = NULL;
 			s->data.plat_entry->list.prev = NULL;
 		}
-- 
2.9.4

[PATCH 7/8] verify: don't use void* for pointer arithmetic (gcc)

[kusumi.tomohiro]

From: Tomohiro Kusumi <tkusumi@tuxera.com>

I think gcc extension should be avoided when it can be done by
just changing a pointer type.

Signed-off-by: Tomohiro Kusumi <tkusumi@tuxera.com>
---
 t/verify-state.c | 3 ++-
 verify-state.h   | 2 +-
 verify.c         | 7 ++++---
 3 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/t/verify-state.c b/t/verify-state.c
index 9a2c3df..78a56da 100644
--- a/t/verify-state.c
+++ b/t/verify-state.c
@@ -58,7 +58,8 @@ static void show(struct thread_io_list *s, size_t size)
 		show_s(s, no_s);
 		no_s++;
 		size -= __thread_io_list_sz(s->depth, s->nofiles);
-		s = (void *) s + __thread_io_list_sz(s->depth, s->nofiles);
+		s = (struct thread_io_list *)((char *) s +
+			__thread_io_list_sz(s->depth, s->nofiles));
 	} while (size != 0);
 }
 
diff --git a/verify-state.h b/verify-state.h
index e46265e..1586f63 100644
--- a/verify-state.h
+++ b/verify-state.h
@@ -77,7 +77,7 @@ static inline size_t thread_io_list_sz(struct thread_io_list *s)
 
 static inline struct thread_io_list *io_list_next(struct thread_io_list *s)
 {
-	return (void *) s + thread_io_list_sz(s);
+	return (struct thread_io_list *)((char *) s + thread_io_list_sz(s));
 }
 
 static inline void verify_state_gen_name(char *out, size_t size,
diff --git a/verify.c b/verify.c
index ffd8707..1f177d7 100644
--- a/verify.c
+++ b/verify.c
@@ -388,7 +388,7 @@ static int verify_io_u_pattern(struct verify_header *hdr, struct vcont *vc)
 	(void)paste_format_inplace(pattern, pattern_size,
 				   td->o.verify_fmt, td->o.verify_fmt_sz, io_u);
 
-	buf = (void *) hdr + header_size;
+	buf = (char *) hdr + header_size;
 	len = get_hdr_inc(td, io_u) - header_size;
 	mod = (get_hdr_inc(td, io_u) * vc->hdr_num + header_size) % pattern_size;
 
@@ -1188,9 +1188,10 @@ static void populate_hdr(struct thread_data *td, struct io_u *io_u,
 			 unsigned int header_len)
 {
 	unsigned int data_len;
-	void *data, *p;
+	void *data;
+	char *p;
 
-	p = (void *) hdr;
+	p = (char *) hdr;
 
 	fill_hdr(td, io_u, hdr, header_num, header_len, io_u->rand_seed);
 
-- 
2.9.4

[PATCH 8/8] smalloc: don't use void* for pointer arithmetic (gcc)

[kusumi.tomohiro]

From: Tomohiro Kusumi <tkusumi@tuxera.com>

I think gcc extension should be avoided when it can be done by
just changing a pointer type.

Signed-off-by: Tomohiro Kusumi <tkusumi@tuxera.com>
---
 smalloc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/smalloc.c b/smalloc.c
index e48cfe8..cab7132 100644
--- a/smalloc.c
+++ b/smalloc.c
@@ -189,7 +189,7 @@ static bool add_pool(struct pool *pool, unsigned int alloc_size)
 		goto out_fail;
 
 	pool->map = ptr;
-	pool->bitmap = (void *) ptr + (pool->nr_blocks * SMALLOC_BPL);
+	pool->bitmap = (unsigned int *)((char *) ptr + (pool->nr_blocks * SMALLOC_BPL));
 	memset(pool->bitmap, 0, bitmap_blocks * sizeof(unsigned int));
 
 	pool->lock = fio_mutex_init(FIO_MUTEX_UNLOCKED);
-- 
2.9.4

Re: [PATCH 1/8] ignore directory= if the given path is absolute

[Jens Axboe]

On 06/23/2017 04:07 PM, kusumi.tomohiro@gmail.com wrote:
> From: Tomohiro Kusumi <tkusumi@tuxera.com>
> 
> When filename= is specified in absolute path format (starts with '/'),
> being able to use directory= doesn't make much sense and also likely
> to result in an unexpected error, thus it should be ignored if any.
> 
> (If combining directory= and absolute filename= is a feature rather
> than unexpected, then it should be documented, since it works as long
> as directory/filename doesn't contain invalid directory. For example,
> directory=. and filename=/tmp/xxx work as ./tmp/xxx if ./tmp exists
> as shown in below.)
> 
> Not sure about the proper way to handle this on Windows, so I had to
> add WIN32 guard.

Honestly, I'd rather just keep it as a strict concatenation of
directory+filename. People could have valid jobs or scripts that
already do variants of

directory=/some_dir
filename=/file_in_some_dir

and it works fine. Your example above is a bit contrived, in my
opinion. If anything, we should improve the error so it shows
the file name fio attempted to use.

-- 
Jens Axboe

Re: [PATCH 2/8] Makefile: use fmt(1) rather than tr(1) on NetBSD/etc

[Jens Axboe]

On 06/23/2017 04:07 PM, kusumi.tomohiro@gmail.com wrote:
> From: Tomohiro Kusumi <tkusumi@tuxera.com>
> 
> e291cff1 (Use fmt -w WIDTH option instead of -WIDTH) changed the
> command option to a common one to suppress an error, but it only
> fixed the unknown option error without being functional on NetBSD.
> 
> This change is taken from NetBSD's pkgsrc. It may work against
> other platforms that don't work with fmt(1), but only enabled for
> NetBSD at the moment. FreeBSD/DragonFlyBSD/OpenBSD work with fmt(1).
> 
> (This actually works on Linux too, but the existing one should be
> kept for Linux (and other platforms) provided it has been used on
> various distros for years)
> 
> https://github.com/NetBSD/pkgsrc/blob/trunk/benchmarks/fio/patches/patch-Makefile
> 
>> Convert the fmt(1) command to a tr(1) one (the fmt(1) old syntax
>> command is not supported on all Unix systems).

Not crazy about the target dependency on netbsd for using tr. But I
guess it's better we have it working.

-- 
Jens Axboe

Re: [PATCH 0/8] option/Makefile fix and void* fixups

[Jens Axboe]

On Sat, Jun 24 2017, kusumi.tomohiro@gmail.com wrote:
> From: Tomohiro Kusumi <tkusumi@tuxera.com>
> 
> 3/8 to 8/8 are the continuation of void* fixups from below,
> but this time in non-ioengine code.
> This is it and no more of this void* patches...
> 
> http://www.spinics.net/lists/fio/msg05912.html
> >   sg: don't use void* for pointer arithmetic (gcc)
> >   splice: don't use void* for pointer arithmetic (gcc)
> >   binject: don't use void* for pointer arithmetic (gcc)
> 
> Tomohiro Kusumi (8):
>   ignore directory= if the given path is absolute
>   Makefile: use fmt(1) rather than tr(1) on NetBSD/etc
>   server: don't use void* for pointer arithmetic (gcc)
>   io_u: don't use void* for pointer arithmetic (gcc)
>   init: don't use void* for pointer arithmetic (gcc)
>   client: don't use void* for pointer arithmetic (gcc)
>   verify: don't use void* for pointer arithmetic (gcc)
>   smalloc: don't use void* for pointer arithmetic (gcc)

Applied 2-8, thanks.

-- 
Jens Axboe

Re: [PATCH 1/8] ignore directory= if the given path is absolute

[Tomohiro Kusumi]

Yes, it's a contrived and minor thing anyway.
I happened to hit this error for having directory=. option left when
filename=/path/to/file was used.


2017-06-24 1:16 GMT+03:00 Jens Axboe <axboe@kernel.dk>:
> On 06/23/2017 04:07 PM, kusumi.tomohiro@gmail.com wrote:
>> From: Tomohiro Kusumi <tkusumi@tuxera.com>
>>
>> When filename= is specified in absolute path format (starts with '/'),
>> being able to use directory= doesn't make much sense and also likely
>> to result in an unexpected error, thus it should be ignored if any.
>>
>> (If combining directory= and absolute filename= is a feature rather
>> than unexpected, then it should be documented, since it works as long
>> as directory/filename doesn't contain invalid directory. For example,
>> directory=. and filename=/tmp/xxx work as ./tmp/xxx if ./tmp exists
>> as shown in below.)
>>
>> Not sure about the proper way to handle this on Windows, so I had to
>> add WIN32 guard.
>
> Honestly, I'd rather just keep it as a strict concatenation of
> directory+filename. People could have valid jobs or scripts that
> already do variants of
>
> directory=/some_dir
> filename=/file_in_some_dir
>
> and it works fine. Your example above is a bit contrived, in my
> opinion. If anything, we should improve the error so it shows
> the file name fio attempted to use.
>
> --
> Jens Axboe
>