Re: [PATCH] device mapper support for strace

["Dmitry V. Levin" <ldv-u2l5PoMzF/Vg9hUCZPvPmw@public.gmane.org>]

[-- Attachment #1.1: Type: text/plain, Size: 6357 bytes --]

On Fri, Jul 31, 2015 at 12:04:48PM -0400, Mikulas Patocka wrote:
> On Fri, 31 Jul 2015, Dmitry V. Levin wrote:
> > On Fri, Jul 31, 2015 at 10:49:44AM -0400, Mikulas Patocka wrote:
> > > Hi
> > > 
> > > Here I'm sending a patch that makes it possible to decode device mapper 
> > > ioctls in strace.
> > > 
> > > How to apply the patch:
> > > 
> > > Download strace 4.10 from 
> > > http://downloads.sourceforge.net/project/strace/strace/4.10/strace-4.10.tar.xz
> > 
> > Please rebase onto strace.git HEAD (v4.10-277-gd9fb450 at this moment).
> > Use ./bootstrap to regenerate dev files.
> 
> This is the updated patch for strace.git.

Thanks.

Unfortunately, this one doesn't apply at all, you must be using a quite
outdated strace.git repository.

The up to date strace.git is located at git://git.code.sf.net/p/strace/code.git,
and when it is not available because of sf.net issues (which are quite
often nowadays), one can use mirrors at https://github.com/ldv-alt/strace.git
and git://git.altlinux.org/people/ldv/public/strace.git

Just a few comments after very cursory look at your patch:

> --- /dev/null
> +++ strace/dm.c
> @@ -0,0 +1,335 @@
> +#include "defs.h"
> +
> +#ifdef HAVE_LINUX_DM_IOCTL_H
> +
> +#include <sys/ioctl.h>

Please include <linux/ioctl.h> instead.

> +#include <linux/dm-ioctl.h>
> +
> +static void
> +dm_decode_device(const unsigned int code, const struct dm_ioctl *ioc)
> +{
> +	if (code != DM_REMOVE_ALL &&
> +	    code != DM_LIST_DEVICES &&
> +	    code != DM_LIST_VERSIONS) {

Please use switch statement instead.

> +		if (ioc->dev)
> +			tprintf(", dev=makedev(%u, %u)", major(ioc->dev), minor(ioc->dev));
> +		if (ioc->name[0]) {
> +			tprints(", name=");
> +			print_quoted_string(ioc->name, DM_NAME_LEN, QUOTE_0_TERMINATED);
> +		}
> +		if (ioc->uuid[0]) {
> +			tprints(", uuid=");
> +			print_quoted_string(ioc->uuid, DM_UUID_LEN, QUOTE_0_TERMINATED);
> +		}
> +	}
> +}
> +
> +static void
> +dm_decode_values(struct tcb *tcp, const unsigned int code, const struct dm_ioctl *ioc)
> +{
> +	if (entering(tcp)) {
> +		if (code == DM_TABLE_LOAD)
> +			tprintf(", target_count=%u", (unsigned)ioc->target_count);
> +		if (code == DM_DEV_RENAME ||
> +		    code == DM_DEV_REMOVE ||
> +		    (code == DM_DEV_SUSPEND && !(ioc->flags & DM_SUSPEND_FLAG)) ||
> +		    code == DM_DEV_WAIT)

Please use switch statement instead.

> +			tprintf(", event_nr=%u", (unsigned)ioc->event_nr);
> +	} else if (!tcp->u_error) {

Please use !syserror(tcp) instead.

> +		if (code == DM_DEV_CREATE ||
> +		    code == DM_DEV_RENAME ||
> +		    code == DM_DEV_SUSPEND ||
> +		    code == DM_DEV_STATUS ||
> +		    code == DM_DEV_WAIT ||
> +		    code == DM_TABLE_LOAD ||
> +		    code == DM_TABLE_CLEAR ||
> +		    code == DM_TABLE_DEPS ||
> +		    code == DM_TABLE_STATUS ||
> +		    code == DM_TARGET_MSG) {

Please use switch statement instead.

> +			tprintf(", target_count=%u", (unsigned)ioc->target_count);
> +			tprintf(", open_count=%u", (unsigned)ioc->open_count);
> +			tprintf(", event_nr=%u", (unsigned)ioc->event_nr);
> +		}
> +	}
> +}
> +
> +static void
> +dm_decode_flags(const struct dm_ioctl *ioc)
> +{
> +	if (ioc->flags) {
> +		static const struct {
> +			uint32_t flag;
> +			const char *string;
> +		} dm_flags[] = {
> +			{ DM_READONLY_FLAG, "DM_READONLY_FLAG" },
> +			{ DM_SUSPEND_FLAG, "DM_SUSPEND_FLAG" },
> +			{ DM_PERSISTENT_DEV_FLAG, "DM_PERSISTENT_DEV_FLAG" },
> +			{ DM_STATUS_TABLE_FLAG, "DM_STATUS_TABLE_FLAG" },
> +			{ DM_ACTIVE_PRESENT_FLAG, "DM_ACTIVE_PRESENT_FLAG" },
> +			{ DM_INACTIVE_PRESENT_FLAG, "DM_INACTIVE_PRESENT_FLAG" },
> +			{ DM_BUFFER_FULL_FLAG, "DM_BUFFER_FULL_FLAG" },
> +			{ DM_SKIP_BDGET_FLAG, "DM_SKIP_BDGET_FLAG" },
> +#ifdef DM_SKIP_LOCKFS_FLAG
> +			{ DM_SKIP_LOCKFS_FLAG, "DM_SKIP_LOCKFS_FLAG" },
> +#endif
> +#ifdef DM_NOFLUSH_FLAG
> +			{ DM_NOFLUSH_FLAG, "DM_NOFLUSH_FLAG" },
> +#endif
> +#ifdef DM_QUERY_INACTIVE_TABLE_FLAG
> +			{ DM_QUERY_INACTIVE_TABLE_FLAG, "DM_QUERY_INACTIVE_TABLE_FLAG" },
> +#endif
> +#ifdef DM_UEVENT_GENERATED_FLAG
> +			{ DM_UEVENT_GENERATED_FLAG, "DM_UEVENT_GENERATED_FLAG" },
> +#endif
> +#ifdef DM_UUID_FLAG
> +			{ DM_UUID_FLAG, "DM_UUID_FLAG" },
> +#endif
> +#ifdef DM_SECURE_DATA_FLAG
> +			{ DM_SECURE_DATA_FLAG, "DM_SECURE_DATA_FLAG" },
> +#endif
> +#ifdef DM_DATA_OUT_FLAG
> +			{ DM_DATA_OUT_FLAG, "DM_DATA_OUT_FLAG" },
> +#endif
> +#ifdef DM_DEFERRED_REMOVE
> +			{ DM_DEFERRED_REMOVE, "DM_DEFERRED_REMOVE" },
> +#endif
> +#ifdef DM_INTERNAL_SUSPEND_FLAG
> +			{ DM_INTERNAL_SUSPEND_FLAG, "DM_INTERNAL_SUSPEND_FLAG" },
> +#endif
> +		};

Please create an xlat file instead.
See e.g. xlat/evdev_*.in files.

> +		uint32_t flags = ioc->flags;
> +		bool first = true;
> +		unsigned i;
> +		tprints(", flags=");
> +		for (i = 0; i < sizeof(dm_flags) / sizeof(*dm_flags); i++) {
> +			if (flags & dm_flags[i].flag) {
> +				if (!first)
> +					tprints("|");
> +				else
> +					first = false;
> +				tprints(dm_flags[i].string);
> +				flags &= ~dm_flags[i].flag;
> +			}
> +		}
> +		if (flags) {
> +			if (!first)
> +				tprints("|");
> +			else
> +				first = false;
> +			tprintf("0x%x", (unsigned)flags);
> +		}

Please use printflags instead.

> +
> +	}
> +}
> +
> +static void
> +dm_decode_dm_target_spec(struct tcb *tcp, const struct dm_ioctl *ioc, const char *extra, size_t extra_size)
> +{
> +	uint32_t i;
> +	size_t offset = ioc->data_start;
> +	for (i = 0; i < ioc->target_count; i++) {
> +		if (offset + sizeof(struct dm_target_spec) >= offset &&
> +		    offset + sizeof(struct dm_target_spec) < extra_size) {
> +			const struct dm_target_spec *s = (const struct dm_target_spec *)(extra + offset);
> +			tprintf(", {sector_start=%llu, length=%llu", (unsigned long long)s->sector_start, (unsigned long long)s->length);
> +			if (!entering(tcp))
> +				tprintf(", status=%d", (int)s->status);
> +			tprints(", target_type=");
> +			print_quoted_string(s->target_type, DM_MAX_TYPE_NAME, QUOTE_0_TERMINATED);
> +			tprints(", string=");
> +			print_quoted_string((const char *)(s + 1), extra_size - (offset + sizeof(struct dm_target_spec)), QUOTE_0_TERMINATED);

Please wrap long lines so they won't exceed 80 symbols.


-- 
ldv

[-- Attachment #1.2: Type: application/pgp-signature, Size: 181 bytes --]

[-- Attachment #2: Type: text/plain, Size: 79 bytes --]

------------------------------------------------------------------------------

[-- Attachment #3: Type: text/plain, Size: 195 bytes --]

_______________________________________________
Strace-devel mailing list
Strace-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
https://lists.sourceforge.net/lists/listinfo/strace-devel