[PATCH 0/3] selftests: ublk: more misc fixes

[PATCH 0/3] selftests: ublk: more misc fixes

[Uday Shankar]

Fix some more minor issues in ublk selftests.

The first patch is from
https://lore.kernel.org/linux-block/20250423-ublk_selftests-v1-0-7d060e260e76@purestorage.com/
with a modification requested by Jens. The others are new.

Signed-off-by: Uday Shankar <ushankar@purestorage.com>
---
Uday Shankar (3):
      selftests: ublk: kublk: build with -Werror iff CONFIG_WERROR=y
      selftests: ublk: make test_generic_06 silent on success
      selftests: ublk: kublk: fix include path

 tools/testing/selftests/ublk/Makefile           | 4 +++-
 tools/testing/selftests/ublk/kublk.h            | 1 -
 tools/testing/selftests/ublk/test_generic_06.sh | 2 +-
 3 files changed, 4 insertions(+), 3 deletions(-)
---
base-commit: 53ec1abce79c986dc59e59d0c60d00088bcdf32a
change-id: 20250428-ublk_selftests-983240d3a325

Best regards,
-- 
Uday Shankar <ushankar@purestorage.com>

[PATCH 1/3] selftests: ublk: kublk: build with -Werror iff CONFIG_WERROR=y

[Uday Shankar]

Compiler warnings can catch bugs at compile time. They can also produce
annoying false positives. Due to this duality, the kernel provides
CONFIG_WERROR so that the developer can choose whether or not they want
compiler warnings to fail the build. Use this same config options to
control whether or not warnings in building kublk fail its build.

Signed-off-by: Uday Shankar <ushankar@purestorage.com>
---
 tools/testing/selftests/ublk/Makefile | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/ublk/Makefile b/tools/testing/selftests/ublk/Makefile
index ec4624a283bce2ebeed80509be6573c1b7a3623d..86474cfe8d03b2df3f8c9bc1a5902701a0f72f58 100644
--- a/tools/testing/selftests/ublk/Makefile
+++ b/tools/testing/selftests/ublk/Makefile
@@ -1,6 +1,8 @@
 # SPDX-License-Identifier: GPL-2.0
 
-CFLAGS += -O3 -Wl,-no-as-needed -Wall -I $(top_srcdir)
+CONFIG = $(top_srcdir)/include/config/auto.conf
+WERROR = $(if $(shell grep CONFIG_WERROR=y ${CONFIG}),-Werror,)
+CFLAGS += -O3 -Wl,-no-as-needed -Wall ${WERROR} -I $(top_srcdir)
 LDLIBS += -lpthread -lm -luring
 
 TEST_PROGS := test_generic_01.sh

-- 
2.34.1

Re: [PATCH 1/3] selftests: ublk: kublk: build with -Werror iff CONFIG_WERROR=y

[Ming Lei]

On Mon, Apr 28, 2025 at 05:10:20PM -0600, Uday Shankar wrote:
> Compiler warnings can catch bugs at compile time. They can also produce
> annoying false positives. Due to this duality, the kernel provides
> CONFIG_WERROR so that the developer can choose whether or not they want
> compiler warnings to fail the build. Use this same config options to
> control whether or not warnings in building kublk fail its build.
> 
> Signed-off-by: Uday Shankar <ushankar@purestorage.com>
> ---
>  tools/testing/selftests/ublk/Makefile | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/testing/selftests/ublk/Makefile b/tools/testing/selftests/ublk/Makefile
> index ec4624a283bce2ebeed80509be6573c1b7a3623d..86474cfe8d03b2df3f8c9bc1a5902701a0f72f58 100644
> --- a/tools/testing/selftests/ublk/Makefile
> +++ b/tools/testing/selftests/ublk/Makefile
> @@ -1,6 +1,8 @@
>  # SPDX-License-Identifier: GPL-2.0
>  
> -CFLAGS += -O3 -Wl,-no-as-needed -Wall -I $(top_srcdir)
> +CONFIG = $(top_srcdir)/include/config/auto.conf
> +WERROR = $(if $(shell grep CONFIG_WERROR=y ${CONFIG}),-Werror,)
> +CFLAGS += -O3 -Wl,-no-as-needed -Wall ${WERROR} -I $(top_srcdir)
>  LDLIBS += -lpthread -lm -luring

I think it isn't good to reuse kernel CONFIG_WERROR for test code.

But it can be done in the following way by passing 'WERROR=1' to make
command line:

+ifneq ($(WERROR),0)
+       CFLAGS += -Werror
+endif



Thanks,
Ming

Re: [PATCH 1/3] selftests: ublk: kublk: build with -Werror iff CONFIG_WERROR=y

[Uday Shankar]

On Tue, Apr 29, 2025 at 09:11:05AM +0800, Ming Lei wrote:
> On Mon, Apr 28, 2025 at 05:10:20PM -0600, Uday Shankar wrote:
> > Compiler warnings can catch bugs at compile time. They can also produce
> > annoying false positives. Due to this duality, the kernel provides
> > CONFIG_WERROR so that the developer can choose whether or not they want
> > compiler warnings to fail the build. Use this same config options to
> > control whether or not warnings in building kublk fail its build.
> > 
> > Signed-off-by: Uday Shankar <ushankar@purestorage.com>
> > ---
> >  tools/testing/selftests/ublk/Makefile | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/tools/testing/selftests/ublk/Makefile b/tools/testing/selftests/ublk/Makefile
> > index ec4624a283bce2ebeed80509be6573c1b7a3623d..86474cfe8d03b2df3f8c9bc1a5902701a0f72f58 100644
> > --- a/tools/testing/selftests/ublk/Makefile
> > +++ b/tools/testing/selftests/ublk/Makefile
> > @@ -1,6 +1,8 @@
> >  # SPDX-License-Identifier: GPL-2.0
> >  
> > -CFLAGS += -O3 -Wl,-no-as-needed -Wall -I $(top_srcdir)
> > +CONFIG = $(top_srcdir)/include/config/auto.conf
> > +WERROR = $(if $(shell grep CONFIG_WERROR=y ${CONFIG}),-Werror,)
> > +CFLAGS += -O3 -Wl,-no-as-needed -Wall ${WERROR} -I $(top_srcdir)
> >  LDLIBS += -lpthread -lm -luring
> 
> I think it isn't good to reuse kernel CONFIG_WERROR for test code.
> 
> But it can be done in the following way by passing 'WERROR=1' to make
> command line:
> 
> +ifneq ($(WERROR),0)
> +       CFLAGS += -Werror
> +endif

I've taken this approach in [1]. It actually passes -Werror by default,
but it gives the developer a way to disable it with

make WERROR=0 TARGETS=ublk kselftest

[1] https://lore.kernel.org/linux-block/20250429-ublk_selftests-v2-1-e970b6d9e4f4@purestorage.com/

[PATCH 2/3] selftests: ublk: make test_generic_06 silent on success

[Uday Shankar]

Convention dictates that tests should not log anything on success. Make
test_generic_06 follow this convention.

Signed-off-by: Uday Shankar <ushankar@purestorage.com>
---
 tools/testing/selftests/ublk/test_generic_06.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/testing/selftests/ublk/test_generic_06.sh b/tools/testing/selftests/ublk/test_generic_06.sh
index b67230c42c847c71b0bbe82ad9de1a737ea3cb75..fd42062b7b76b0b3dfae95a39aba6ae28facc185 100755
--- a/tools/testing/selftests/ublk/test_generic_06.sh
+++ b/tools/testing/selftests/ublk/test_generic_06.sh
@@ -17,7 +17,7 @@ STARTTIME=${SECONDS}
 dd if=/dev/urandom of=/dev/ublkb${dev_id} oflag=direct bs=4k count=1 status=none > /dev/null 2>&1 &
 dd_pid=$!
 
-__ublk_kill_daemon ${dev_id} "DEAD"
+__ublk_kill_daemon ${dev_id} "DEAD" >/dev/null
 
 wait $dd_pid
 dd_exitcode=$?

-- 
2.34.1

Re: [PATCH 2/3] selftests: ublk: make test_generic_06 silent on success

[Ming Lei]

On Mon, Apr 28, 2025 at 05:10:21PM -0600, Uday Shankar wrote:
> Convention dictates that tests should not log anything on success. Make
> test_generic_06 follow this convention.
> 
> Signed-off-by: Uday Shankar <ushankar@purestorage.com>

Reviewed-by: Ming Lei <ming.lei@redhat.com>


Thanks,
Ming

[PATCH 3/3] selftests: ublk: kublk: fix include path

[Uday Shankar]

Building kublk currently fails (with a "could not find linux/ublk_cmd.h"
error message) if kernel headers are not installed in a system-global
location (i.e. somewhere in the compiler's default include search path).
This failure is unnecessary, as make kselftest installs kernel headers
in the build tree - kublk's build just isn't looking for them properly.
There is an include path in kublk's CFLAGS which is probably intended to
find the kernel headers installed in the build tree; fix it so that it
can actually find them.

This introduces some macro redefinition issues between glibc-provided
headers and kernel headers; fix those by eliminating one include in
kublk.

Signed-off-by: Uday Shankar <ushankar@purestorage.com>
---
 tools/testing/selftests/ublk/Makefile | 2 +-
 tools/testing/selftests/ublk/kublk.h  | 1 -
 2 files changed, 1 insertion(+), 2 deletions(-)

diff --git a/tools/testing/selftests/ublk/Makefile b/tools/testing/selftests/ublk/Makefile
index 86474cfe8d03b2df3f8c9bc1a5902701a0f72f58..feca641635d706a856898f8e22a630f5f47361b6 100644
--- a/tools/testing/selftests/ublk/Makefile
+++ b/tools/testing/selftests/ublk/Makefile
@@ -2,7 +2,7 @@
 
 CONFIG = $(top_srcdir)/include/config/auto.conf
 WERROR = $(if $(shell grep CONFIG_WERROR=y ${CONFIG}),-Werror,)
-CFLAGS += -O3 -Wl,-no-as-needed -Wall ${WERROR} -I $(top_srcdir)
+CFLAGS += -O3 -Wl,-no-as-needed -Wall ${WERROR} -I $(top_srcdir)/usr/include
 LDLIBS += -lpthread -lm -luring
 
 TEST_PROGS := test_generic_01.sh
diff --git a/tools/testing/selftests/ublk/kublk.h b/tools/testing/selftests/ublk/kublk.h
index 918db5cd633fc1041e1e0805142f00e7e4f28bf7..4b943e57a890e5f14fa11dd19d67c4c8684c4417 100644
--- a/tools/testing/selftests/ublk/kublk.h
+++ b/tools/testing/selftests/ublk/kublk.h
@@ -19,7 +19,6 @@
 #include <sys/inotify.h>
 #include <sys/wait.h>
 #include <sys/eventfd.h>
-#include <sys/uio.h>
 #include <sys/ipc.h>
 #include <sys/shm.h>
 #include <linux/io_uring.h>

-- 
2.34.1

Re: [PATCH 3/3] selftests: ublk: kublk: fix include path

[Caleb Sander Mateos]

On Mon, Apr 28, 2025 at 4:11 PM Uday Shankar <ushankar@purestorage.com> wrote:
>
> Building kublk currently fails (with a "could not find linux/ublk_cmd.h"
> error message) if kernel headers are not installed in a system-global
> location (i.e. somewhere in the compiler's default include search path).
> This failure is unnecessary, as make kselftest installs kernel headers
> in the build tree - kublk's build just isn't looking for them properly.
> There is an include path in kublk's CFLAGS which is probably intended to
> find the kernel headers installed in the build tree; fix it so that it
> can actually find them.
>
> This introduces some macro redefinition issues between glibc-provided
> headers and kernel headers; fix those by eliminating one include in
> kublk.

I'm curious what symbol was redefined. struct iovec? Anyways,

Reviewed-by: Caleb Sander Mateos <csander@purestorage.com>

>
> Signed-off-by: Uday Shankar <ushankar@purestorage.com>
> ---
>  tools/testing/selftests/ublk/Makefile | 2 +-
>  tools/testing/selftests/ublk/kublk.h  | 1 -
>  2 files changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/tools/testing/selftests/ublk/Makefile b/tools/testing/selftests/ublk/Makefile
> index 86474cfe8d03b2df3f8c9bc1a5902701a0f72f58..feca641635d706a856898f8e22a630f5f47361b6 100644
> --- a/tools/testing/selftests/ublk/Makefile
> +++ b/tools/testing/selftests/ublk/Makefile
> @@ -2,7 +2,7 @@
>
>  CONFIG = $(top_srcdir)/include/config/auto.conf
>  WERROR = $(if $(shell grep CONFIG_WERROR=y ${CONFIG}),-Werror,)
> -CFLAGS += -O3 -Wl,-no-as-needed -Wall ${WERROR} -I $(top_srcdir)
> +CFLAGS += -O3 -Wl,-no-as-needed -Wall ${WERROR} -I $(top_srcdir)/usr/include
>  LDLIBS += -lpthread -lm -luring
>
>  TEST_PROGS := test_generic_01.sh
> diff --git a/tools/testing/selftests/ublk/kublk.h b/tools/testing/selftests/ublk/kublk.h
> index 918db5cd633fc1041e1e0805142f00e7e4f28bf7..4b943e57a890e5f14fa11dd19d67c4c8684c4417 100644
> --- a/tools/testing/selftests/ublk/kublk.h
> +++ b/tools/testing/selftests/ublk/kublk.h
> @@ -19,7 +19,6 @@
>  #include <sys/inotify.h>
>  #include <sys/wait.h>
>  #include <sys/eventfd.h>
> -#include <sys/uio.h>
>  #include <sys/ipc.h>
>  #include <sys/shm.h>
>  #include <linux/io_uring.h>
>
> --
> 2.34.1
>
>

Re: [PATCH 3/3] selftests: ublk: kublk: fix include path

[Uday Shankar]

On Mon, Apr 28, 2025 at 06:14:18PM -0700, Caleb Sander Mateos wrote:
> On Mon, Apr 28, 2025 at 4:11 PM Uday Shankar <ushankar@purestorage.com> wrote:
> >
> > Building kublk currently fails (with a "could not find linux/ublk_cmd.h"
> > error message) if kernel headers are not installed in a system-global
> > location (i.e. somewhere in the compiler's default include search path).
> > This failure is unnecessary, as make kselftest installs kernel headers
> > in the build tree - kublk's build just isn't looking for them properly.
> > There is an include path in kublk's CFLAGS which is probably intended to
> > find the kernel headers installed in the build tree; fix it so that it
> > can actually find them.
> >
> > This introduces some macro redefinition issues between glibc-provided
> > headers and kernel headers; fix those by eliminating one include in
> > kublk.
> 
> I'm curious what symbol was redefined. struct iovec? Anyways,

The RWF_* flags. Here's an example error:

In file included from /root/linux/tools/testing/selftests/../../../usr/include/linux/io_uring.h:11,
                 from kublk.h:25,
                 from kublk.c:6:
/root/linux/tools/testing/selftests/../../../usr/include/linux/fs.h:318: error: "RWF_HIPRI" redefined [-Werror]
  318 | #define RWF_HIPRI       ((__kernel_rwf_t)0x00000001)
      |
In file included from /usr/include/sys/uio.h:179,
                 from kublk.h:22,
                 from kublk.c:6:
/usr/include/bits/uio-ext.h:45: note: this is the location of the previous definition
   45 | #define RWF_HIPRI       0x00000001 /* High priority request.  */
      |

Re: [PATCH 3/3] selftests: ublk: kublk: fix include path

[Ming Lei]

On Mon, Apr 28, 2025 at 05:10:22PM -0600, Uday Shankar wrote:
> Building kublk currently fails (with a "could not find linux/ublk_cmd.h"
> error message) if kernel headers are not installed in a system-global
> location (i.e. somewhere in the compiler's default include search path).
> This failure is unnecessary, as make kselftest installs kernel headers
> in the build tree - kublk's build just isn't looking for them properly.
> There is an include path in kublk's CFLAGS which is probably intended to
> find the kernel headers installed in the build tree; fix it so that it
> can actually find them.
> 
> This introduces some macro redefinition issues between glibc-provided
> headers and kernel headers; fix those by eliminating one include in
> kublk.
> 
> Signed-off-by: Uday Shankar <ushankar@purestorage.com>

kernel selftest is supposed to run under the kernel tree without installing
headers system wide, nice!

Reviewed-by: Ming Lei <ming.lei@redhat.com>



Thanks,
Ming

Re: [PATCH 0/3] selftests: ublk: more misc fixes

[Uday Shankar]

Hi Jens,

Can you take a look at Ming's comment on the first patch and merge the
set if things look good? I can rebase/repost it as needed.

Thanks,
Uday

Re: [PATCH 0/3] selftests: ublk: more misc fixes

[Uday Shankar]

On Mon, May 05, 2025 at 02:31:40PM -0600, Uday Shankar wrote:
> Hi Jens,
> 
> Can you take a look at Ming's comment on the first patch and merge the
> set if things look good? I can rebase/repost it as needed.

Bleh, sorry, I meant to send this as a reply to v2:

https://lore.kernel.org/linux-block/20250429-ublk_selftests-v2-0-e970b6d9e4f4@purestorage.com/

Re: [PATCH 0/3] selftests: ublk: more misc fixes

[Jens Axboe]

On 5/5/25 2:34 PM, Uday Shankar wrote:
> On Mon, May 05, 2025 at 02:31:40PM -0600, Uday Shankar wrote:
>> Hi Jens,
>>
>> Can you take a look at Ming's comment on the first patch and merge the
>> set if things look good? I can rebase/repost it as needed.
> 
> Bleh, sorry, I meant to send this as a reply to v2:
> 
> https://lore.kernel.org/linux-block/20250429-ublk_selftests-v2-0-e970b6d9e4f4@purestorage.com/

Let's give Ming a chance to review v2, then I can get it queued up.

-- 
Jens Axboe

Re: [PATCH 0/3] selftests: ublk: more misc fixes

[Uday Shankar]

On Mon, May 05, 2025 at 04:44:19PM -0600, Jens Axboe wrote:
> On 5/5/25 2:34 PM, Uday Shankar wrote:
> > On Mon, May 05, 2025 at 02:31:40PM -0600, Uday Shankar wrote:
> >> Hi Jens,
> >>
> >> Can you take a look at Ming's comment on the first patch and merge the
> >> set if things look good? I can rebase/repost it as needed.
> > 
> > Bleh, sorry, I meant to send this as a reply to v2:
> > 
> > https://lore.kernel.org/linux-block/20250429-ublk_selftests-v2-0-e970b6d9e4f4@purestorage.com/
> 
> Let's give Ming a chance to review v2, then I can get it queued up.

It looks like he has already reviewed all the patches in the set.

Re: [PATCH 0/3] selftests: ublk: more misc fixes

[Jens Axboe]

On 5/5/25 4:53 PM, Uday Shankar wrote:
> On Mon, May 05, 2025 at 04:44:19PM -0600, Jens Axboe wrote:
>> On 5/5/25 2:34 PM, Uday Shankar wrote:
>>> On Mon, May 05, 2025 at 02:31:40PM -0600, Uday Shankar wrote:
>>>> Hi Jens,
>>>>
>>>> Can you take a look at Ming's comment on the first patch and merge the
>>>> set if things look good? I can rebase/repost it as needed.
>>>
>>> Bleh, sorry, I meant to send this as a reply to v2:
>>>
>>> https://lore.kernel.org/linux-block/20250429-ublk_selftests-v2-0-e970b6d9e4f4@purestorage.com/
>>
>> Let's give Ming a chance to review v2, then I can get it queued up.
> 
> It looks like he has already reviewed all the patches in the set.

You're right, was looking at the other v2 that you posted today.
Now queued up!

-- 
Jens Axboe