Suggestion for patch

[Dmitry V. Levin]

Hi,

On Thu, Feb 07, 2013 at 03:15:17PM +0000, Frediano Ziglio wrote:
>   I was trying to add support to Xen ioctl to strace. To distinguish
> from other ioctl I used the entire code (not only type and number but
> also size and direction) so I changed sys_ioctl in io.c to call a
> function that check for the entire code and return true if it handled
> the syscall. Is it fine to do it?

It is probably OK, at least I couldn't think up a better alternative yet.

> The ioctl of Xen are quite complicated to decode cause there are single
> ioctls that accept a structure that internally have a command field that
> specify the operation and an union with all data for operations,
> something like
> 
> struct my_operation {
>   int operation;
>   union {
>     struct operation1_data operation1;
>     struct operation2_data operation2;
>     struct operation3_data operation3;
>     ...
>   } u;
> };
> 
> Just to complicate more some field in operationX_data can be for output,
> input or input/output. I found no way to store some informations in tcb
> structure. I'd like to print something when syscall enters (input
> parameters) and something when syscall leaves, somethings like
> 
> ioctl(FD, IOCTL, {op=my_op,u={field_inout=5}}->{u={field_inout=3}}) = 0
> 
> (->) to separate input and output. Is there a  specific syntax to
> separate input and output?

This is uncharted territory, we had nothing like this before.
Anyway, a space before and after this delimiter wouldn't harm.

> +extern int ioctl_decode_exact(struct tcb *tcp);

A declaration would have to go to defs.h

>  int
>  sys_ioctl(struct tcb *tcp)
>  {
> @@ -384,6 +386,12 @@ sys_ioctl(struct tcb *tcp)
>  	if (entering(tcp)) {
>  		printfd(tcp, tcp->u_arg[0]);
>  		tprints(", ");
> +	}
> +
> +	if (ioctl_decode_exact(tcp))
> +		return 0;
> +
> +	if (entering(tcp)) {
>  		iop = ioctl_lookup(tcp->u_arg[1]);
>  		if (iop) {
>  			tprints(iop->symbol);

I'd rather put this ioctl_decode_exact() call to both branches.

> +#include <xenctrl.h>
> +#include <xen/sys/evtchn.h>
> +#include <xen/sys/privcmd.h>

Availability of these headers has to be checked in configure.ac so that
HAVE_XENCTRL_H and other macros could be used here.

I'd prefer to put this complicated xen ioctl parser to separate xen.c
source file.

> +// hold information on status
> +// need flush if space is not enough
> +typedef struct {
> +	char *dest, *dest_end;
> +	char buffer[256];
> +} print_status;
> +
> +static inline void
> +status_init(print_status *status)
> +{
> +	status->dest     = status->buffer;
> +	status->dest_end = status->buffer + sizeof(status->buffer);
> +}
> +
> +static void
> +assure_space(print_status *status, size_t needed)
> +{
> +	/* assert(needed <= sizeof(status->buffer)); */
> +	if (needed > status->dest_end - status->dest) {
> +		tprints(status->buffer);
> +		status->buffer[0] = 0;
> +		status->dest = status->buffer;
> +	}
> +}
> +
> +static inline void
> +status_flush(print_status *status)
> +{
> +	assure_space(status, sizeof(status->buffer));
> +}
> +
> +static void
> +status_printf(print_status *status, const char *fmt, ...)
> +{
> +	int l;
> +	va_list ap;
> +
> +	assure_space(status, 64);
> +	va_start(ap, fmt);
> +	l = vsprintf(status->dest, fmt, ap);
> +	va_end(ap);
> +	status->dest += l;
> +}

Why do we need additional buffering on top of tprintf/tprints?
Does the latter perform so badly?


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