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 <e...@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
signature.asc
Description: PGP signature
------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________ Strace-devel mailing list Strace-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/strace-devel
