[RFC/PATCH v3 1/2] watchdog_ioctl: Add decoding of WDIOC_GETSUPPORT and WDIOC_SETOPTIONS ioctls

[Eugene Syromyatnikov]

On Thu, Aug 08, 2024 at 01:29:55AM +0530, Sahil Siddiq wrote:
> * src/watchdog_ioctl.c: Implement new ioctl decoders.
> * src/xlat/watchdog_ioctl_cmds.in (WDIOC_GETSUPPORT): Add new constant.
> * src/xlat/watchdog_ioctl_setoptions.in: New file.
> * src/xlat/watchdog_ioctl_support_options.in: New file.
> * NEWS: Mention this change.
> 
> Signed-off-by: Sahil Siddiq <sahilcdq at proton.me>

Thank you for working on this.  A couple of nits is below.

> ---
> Changes v1 -> v3:
> - src/watchdog_ioctl.c:
>   (options): Change to "unsigned int".
>   (identity): Print using PRINT_FIELD_CSTRING.
>   (WDIOC_SETOPTIONS):
>   - Shift "tprint_arg_next" above umove.
>   - Use "printflags" instead of its 64-bit counterpart.
> 
> Changes v2 -> v3:
> - Move tests to commit #2.
> 
>  NEWS                                       |  2 ++
>  src/watchdog_ioctl.c                       | 32 ++++++++++++++++++++++
>  src/xlat/watchdog_ioctl_cmds.in            |  1 +
>  src/xlat/watchdog_ioctl_setoptions.in      |  3 ++
>  src/xlat/watchdog_ioctl_support_options.in | 12 ++++++++
>  5 files changed, 50 insertions(+)
>  create mode 100644 src/xlat/watchdog_ioctl_setoptions.in
>  create mode 100644 src/xlat/watchdog_ioctl_support_options.in
> 
> diff --git a/NEWS b/NEWS
> index b3bc8e932..cf1466c7e 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -6,6 +6,8 @@ Noteworthy changes in release ?.?? (????-??-??)
>    * Updated decoding of listmount, statmount, and statx syscalls.
>    * Updated lists of ETHTOOL_*, IORING_*, IPPROTO_*, RWF_*, STATX_*, and V4L2_*
>      constants.
> +  * Implemented decoding of WDIOC_GETSUPPORT and WDIOC_SETOPTIONS ioctl
> +    commands.
>  
>  Noteworthy changes in release 6.10 (2024-07-21)
>  ===============================================
> diff --git a/src/watchdog_ioctl.c b/src/watchdog_ioctl.c
> index b88094f0d..5f9b1824a 100644
> --- a/src/watchdog_ioctl.c
> +++ b/src/watchdog_ioctl.c
> @@ -9,6 +9,9 @@
>  
>  #include <linux/watchdog.h>
>  
> +#include "xlat/watchdog_ioctl_setoptions.h"

> +#include "xlat/watchdog_ioctl_support_options.h"

I'd rather name this one "watchdog_ioctl_flags", they are seemingly
used in WDIOC_GET{,BOOT}STATUS as well.

> +
>  #define XLAT_MACROS_ONLY
>  #include "xlat/watchdog_ioctl_cmds.h"
>  #undef XLAT_MACROS_ONLY
> @@ -17,7 +20,29 @@ int
>  watchdog_ioctl(struct tcb *const tcp, const unsigned int code,
>  	   const kernel_ulong_t arg)
>  {
> +	struct watchdog_info ident;
> +	unsigned int options;
> +
>  	switch (code) {
> +	case WDIOC_GETSUPPORT:
> +		if (entering(tcp))
> +			return 0;
> +		tprint_arg_next();

It's preferred to first call tprint_arg_next, and only then turn, so it
looks nicer when a tracee switch occurs, like so:

	if (entering(tcp)) {
		tprint_arg_next();
		return 0;
	}

So it yields "ioctl(..., <unfinished ...>>".

> +		if (!umove_or_printaddr(tcp, arg, &ident)) {

You can opt for

	if (umove_or_printaddr(tcp, arg, &ident))
		break;

here to avoid indenting the successful read case.

> +			tprint_struct_begin();
> +			PRINT_FIELD_FLAGS(ident, options, watchdog_ioctl_support_options,

An overly long line.

> +					  "WDIOF_???");
> +			tprint_struct_next();

> +			if (abbrev(tcp))
> +				tprint_more_data_follows();
> +			else {

It's a bit nicer (and safer for possible future changes) to have
the curly braces for the if branch as well.

> +				PRINT_FIELD_U(ident, firmware_version);

I wonder if PRINT_FIELD_X makes more sense here—sometimes developers
tend to encode major/minor revisions as multiples of 16.  Do you have
any insight in what actually appears here in the wild, by chance?

> +				tprint_struct_next();
> +				PRINT_FIELD_CSTRING(ident, identity);
> +			}
> +			tprint_struct_end();
> +		}
> +		break;
>  	case WDIOC_GETSTATUS:
>  	case WDIOC_GETBOOTSTATUS:
>  	case WDIOC_GETTEMP:
> @@ -32,6 +57,13 @@ watchdog_ioctl(struct tcb *const tcp, const unsigned int code,
>  		tprint_arg_next();
>  		printnum_int(tcp, arg, "%d");
>  		break;
> +	case WDIOC_SETOPTIONS:
> +		tprint_arg_next();
> +		if (!umove_or_printaddr(tcp, arg, &options)) {
> +			tprint_indirect_begin();

> +			printflags(watchdog_ioctl_setoptions, options, "WDIOS_???");

An overly long line.

> +			tprint_indirect_end();
> +		}
>  
>  	/*
>  	 * linux/watchdog.h says that this takes an int, but in
> diff --git a/src/xlat/watchdog_ioctl_cmds.in b/src/xlat/watchdog_ioctl_cmds.in
> index b871cbd1e..d77f3d41c 100644
> --- a/src/xlat/watchdog_ioctl_cmds.in
> +++ b/src/xlat/watchdog_ioctl_cmds.in
> @@ -1,3 +1,4 @@
> +WDIOC_GETSUPPORT        _IOR('W', 0, struct watchdog_info)
>  WDIOC_GETSTATUS         _IOR('W', 1, int)
>  WDIOC_GETBOOTSTATUS     _IOR('W', 2, int)
>  WDIOC_GETTEMP           _IOR('W', 3, int)
> diff --git a/src/xlat/watchdog_ioctl_setoptions.in b/src/xlat/watchdog_ioctl_setoptions.in
> new file mode 100644
> index 000000000..5ef67714e
> --- /dev/null
> +++ b/src/xlat/watchdog_ioctl_setoptions.in
> @@ -0,0 +1,3 @@
> +WDIOS_DISABLECARD      0x0001
> +WDIOS_ENABLECARD       0x0002
> +WDIOS_TEMPPANIC        0x0004
> diff --git a/src/xlat/watchdog_ioctl_support_options.in b/src/xlat/watchdog_ioctl_support_options.in
> new file mode 100644
> index 000000000..2030cf329
> --- /dev/null
> +++ b/src/xlat/watchdog_ioctl_support_options.in
> @@ -0,0 +1,12 @@
> +WDIOF_OVERHEAT          0x0001
> +WDIOF_FANFAULT          0x0002
> +WDIOF_EXTERN1           0x0004
> +WDIOF_EXTERN2           0x0008
> +WDIOF_POWERUNDER        0x0010
> +WDIOF_CARDRESET         0x0020
> +WDIOF_POWEROVER         0x0040
> +WDIOF_SETTIMEOUT        0x0080
> +WDIOF_MAGICCLOSE        0x0100
> +WDIOF_PRETIMEOUT        0x0200
> +WDIOF_ALARMONLY         0x0400
> +WDIOF_KEEPALIVEPING     0x8000
> -- 
> 2.45.2
> 
> -- 
> Strace-devel mailing list
> Strace-devel at lists.strace.io
> https://lists.strace.io/mailman/listinfo/strace-devel