[PATCH 2/3] v4l2: printings for all buf types

Dmitry V. Levin ldv at altlinux.org
Wed Mar 29 00:06:08 UTC 2017


On Tue, Mar 28, 2017 at 06:21:29PM +0300, Edgar Kaziahmedov wrote:
> The commit adds meaningful and informative printings for all
> v4l2 buf types in the 'print_v4l2_format_fmt' routine.
> 
> Signed-off-by: Edgar Kaziahmedov <edos at linux.com>
> ---
>  v4l2.c | 110 +++++++++++++++++++++++++++++++++++++++++++++++++++--------------
>  1 file changed, 86 insertions(+), 24 deletions(-)
> 
> diff --git a/v4l2.c b/v4l2.c
> index 637e8788..c7127961 100644
> --- a/v4l2.c
> +++ b/v4l2.c
> @@ -51,6 +51,7 @@ typedef struct v4l2_format struct_v4l2_format;
>  typedef struct v4l2_framebuffer struct_v4l2_framebuffer;
>  typedef struct v4l2_input struct_v4l2_input;
>  typedef struct v4l2_standard struct_v4l2_standard;
> +typedef struct v4l2_clip struct_v4l2_clip;

As struct v4l2_clip contains pointers, it has to be mpers'ed indeed.
The typedef is one half of the thing, another half is DEF_MPERS_TYPE.

Also, please keep this list sorted.

>  #include MPERS_DEFS
>  
> @@ -200,10 +201,22 @@ print_v4l2_fmtdesc(struct tcb *const tcp, const kernel_ulong_t arg)
>  
>  #include "xlat/v4l2_fields.h"
>  #include "xlat/v4l2_colorspaces.h"
> +#include "xlat/v4l2_vbi_flags.h"
> +#include "xlat/v4l2_sliced_flags.h"
>  
> -static void
> -print_v4l2_format_fmt(const char *prefix, const struct_v4l2_format *f)
> +static bool
> +print_v4l2_clip(struct tcb *tcp, void *elem_buf, size_t elem_size, void* data)
> +{
> +	const struct_v4l2_clip *p = elem_buf;
> +	tprintf(FMT_RECT, ARGS_RECT(p->c));
> +	return true;
> +}
> +
> +static bool
> +print_v4l2_format_fmt(struct tcb *const tcp, const char *prefix,
> +		      const struct_v4l2_format *f)
>  {
> +	bool ret = true;
>  	switch (f->type) {
>  	case V4L2_BUF_TYPE_VIDEO_CAPTURE:
>  	case V4L2_BUF_TYPE_VIDEO_OUTPUT:
> @@ -220,8 +233,8 @@ print_v4l2_format_fmt(const char *prefix, const struct_v4l2_format *f)
>  		tprints("}");
>  		break;
>  #if HAVE_DECL_V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE
> -	case V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE:
> -	case V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE: {
> +	case V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE:
> +	case V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE: {
>  		unsigned int i, max;
>  
>  		tprints(prefix);

Why do you need to reorder them?
Please avoid irrelevant changes.

> @@ -244,42 +257,89 @@ print_v4l2_format_fmt(const char *prefix, const struct_v4l2_format *f)
>  				f->fmt.pix_mp.plane_fmt[i].sizeimage,
>  				f->fmt.pix_mp.plane_fmt[i].bytesperline);
>  		}
> -		tprintf("], num_planes=%u}", (unsigned) f->fmt.pix_mp.num_planes);
> +		tprintf("], num_planes=%u}",
> +			(unsigned) f->fmt.pix_mp.num_planes);
>  		break;
>  	}
>  #endif
> -
> -	/* TODO: Complete this switch statement */
> -#if 0
> -	case V4L2_BUF_TYPE_VIDEO_OVERLAY:
>  #if HAVE_DECL_V4L2_BUF_TYPE_VIDEO_OUTPUT_OVERLAY
> -	case V4L2_BUF_TYPE_VIDEO_OUTPUT_OVERLAY:
> -#endif
> +	case V4L2_BUF_TYPE_VIDEO_OVERLAY:
> +	case V4L2_BUF_TYPE_VIDEO_OUTPUT_OVERLAY: {

V4L2_BUF_TYPE_VIDEO_OUTPUT_OVERLAY was added in the kernel after
V4L2_BUF_TYPE_VIDEO_OVERLAY; why do you penalising the latter?

> +		struct_v4l2_clip clip;
>  		tprints(prefix);
> -		tprints("fmt.win={???}");
> +		tprintf("fmt.win={left=%d, top=%d, width=%u, height=%u, field=",
> +			ARGS_RECT(f->fmt.win.w));
> +		printxval(v4l2_fields, f->fmt.win.field, "V4L2_FIELD_???");
> +		tprintf(", chromakey=%#x, clips=", f->fmt.win.chromakey);
> +		ret = print_array(tcp, ptr_to_kulong(f->fmt.win.clips),
> +				  f->fmt.win.clipcount, &clip, sizeof(clip),
> +				  umoven_or_printaddr_ignore_syserror,

Is this use of umoven_or_printaddr_ignore_syserror instead of regular
umoven_or_printaddr justified?  Do you really want to print v4l2_clip
on exiting of a failed ioctl syscall?

> +				  print_v4l2_clip, 0);
> +		tprintf(", clipcount=%u, bitmap=", f->fmt.win.clipcount);
> +		printaddr(ptr_to_kulong(f->fmt.win.bitmap));
> +		tprintf(", global_alpha=%#x}", f->fmt.win.global_alpha);
>  		break;
> -
> +	}
> +#endif
> +#if HAVE_DECL_V4L2_BUF_TYPE_VBI_CAPTURE
>  	case V4L2_BUF_TYPE_VBI_CAPTURE:
>  	case V4L2_BUF_TYPE_VBI_OUTPUT:
>  		tprints(prefix);
> -		tprints("fmt.vbi={???}");
> +		tprintf("fmt.vbi={sampling_rate=%u, offset=%u, "
> +			"samples_per_line=%u, sample_format=",
> +			f->fmt.vbi.sampling_rate, f->fmt.vbi.offset,
> +			f->fmt.vbi.samples_per_line);
> +		print_pixelformat(f->fmt.vbi.sample_format);
> +		tprintf(", start=%u,%u, count=%u,%u, ",
> +			f->fmt.vbi.start[0], f->fmt.vbi.start[1],
> +			f->fmt.vbi.count[0], f->fmt.vbi.count[1]);

%u,%u describes no C type, please change to [%u, %u].

> +		tprints("flags=");
> +		printxval(v4l2_vbi_flags, f->fmt.vbi.flags, "V4L2_VBI_???");
> +		tprintf(", reserved=%#x,%#x}", f->fmt.vbi.reserved[0],
> +			f->fmt.vbi.reserved[1]);

Do you think printing these reserved fields is useful?

>  		break;
> -
> +#endif
> +#if HAVE_DECL_V4L2_BUF_TYPE_SLICED_VBI_CAPTURE
>  	case V4L2_BUF_TYPE_SLICED_VBI_CAPTURE:
> -	case V4L2_BUF_TYPE_SLICED_VBI_OUTPUT:
> +	case V4L2_BUF_TYPE_SLICED_VBI_OUTPUT: {
> +		unsigned int i;
> +
>  		tprints(prefix);
> -		tprints("fmt.sliced={???}");
> -		break;
> +		tprints("fmt.sliced={service_set=");
> +		printxval(v4l2_sliced_flags, f->fmt.sliced.service_set,
> +			"V4L2_SLICED_???");
> +		tprintf(", io_size=%u, service_lines=[",
> +			f->fmt.sliced.io_size);
> +		/* I'm sorry for this magic constant, but standard headers
> +		 * doesn't contain the corresponding define for
> +		 * v4l2_sliced_vbi_format
> +		*/
> +		for (i = 0; i < 24; i++) {

Please use ARRAY_SIZE(f->fmt.sliced.service_lines[0]) instead.

> +			if (i > 0)
> +				tprints(", ");
> +			tprintf("{line[%u]=%#x, %#x}", i,
> +				f->fmt.sliced.service_lines[0][i],
> +				f->fmt.sliced.service_lines[1][i]);

{line[%u]=%#x, %#x} is not a C type.

> +		}
> +		tprintf("], reserved=%#x,%#x}",
> +			f->fmt.sliced.reserved[0],
> +			f->fmt.sliced.reserved[1]);
>  
> +		break;
> +	}
> +#endif
>  #if HAVE_DECL_V4L2_BUF_TYPE_SDR_CAPTURE
>  	case V4L2_BUF_TYPE_SDR_CAPTURE:
>  	case V4L2_BUF_TYPE_SDR_OUTPUT:
>  		tprints(prefix);
> -		tprints("fmt.sdr={???}");
> +		tprints("fmt.sdr={pixelformat=");
> +		print_pixelformat(f->fmt.sdr.pixelformat);
> +		tprintf(", buffersize=%u}",
> +			f->fmt.sdr.buffersize);
>  		break;
>  #endif
> -#endif
>  	}
> +	return ret;
>  }
>  
>  static int
> @@ -287,7 +347,7 @@ print_v4l2_format(struct tcb *const tcp, const kernel_ulong_t arg,
>  		  const bool is_get)
>  {
>  	struct_v4l2_format f;
> -

Please keep this empty line.

> +	bool fail = false;

I don't think you need it, see below.

>  	if (entering(tcp)) {
>  		tprints(", ");
>  		if (umove_or_printaddr(tcp, arg, &f))
> @@ -296,14 +356,16 @@ print_v4l2_format(struct tcb *const tcp, const kernel_ulong_t arg,
>  		printxval(v4l2_buf_types, f.type, "V4L2_BUF_TYPE_???");
>  		if (is_get)
>  			return 0;
> -		print_v4l2_format_fmt(", ", &f);
> +		fail = !print_v4l2_format_fmt(tcp, ", ", &f);

RVAL_DECODED | 1 can be returned at this point if print_v4l2_format_fmt
indicated a failure, but see below.

>  	} else {
>  		if (!syserror(tcp) && !umove(tcp, arg, &f)) {
>  			const char *delim = is_get ? ", " : " => ";
> -			print_v4l2_format_fmt(delim, &f);
> +			fail = !print_v4l2_format_fmt(tcp, delim, &f);

On exiting syscall this affects nothing.

>  		}
>  		tprints("}");
>  	}
> +	if (fail)
> +		return RVAL_DECODED | 1;

If this happened on entering syscall, an invalid structure
without closing "}" will be printed.

>  	return 1;
>  }
>  

-- 
ldv
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: <http://lists.strace.io/pipermail/strace-devel/attachments/20170329/eb622f71/attachment.bin>


More information about the Strace-devel mailing list