Re: [PATCH] device mapper support for strace

[Mike F. <va...@ge...>]

On 11 Aug 2015 13:11, Mikulas Patocka wrote:
> --- /dev/null
> +++ strace/dm.c
> @@ -0,0 +1,354 @@
> +#include "defs.h"

all files should have a comment block at the top.  see mtd.c as an example.

> +{
> +	switch (code) {
> +		case DM_REMOVE_ALL:

case statements should be at the same indent level as the switch.  see mtd.c.

> +	size_t offset = ioc->data_start;

data_start is a __u32, so this should be uint32_t instead of size_t

> +			tprintf(", {sector_start=%llu, length=%llu",
> +				(unsigned long long)s->sector_start,
> +				(unsigned long long)s->length);

sector_start/length are uint64_t, not unsigned long long.  please use PRIu64 
defines from inttypes.h instead of random casts.  you can find plenty of 
examples in the tree already.

> +			if (!entering(tcp))
> +				tprintf(", status=%d", (int)s->status);

status is int32_t, not int, so please use PRId32 instead of int casts.

> +static void
> +dm_decode_dm_target_deps(const struct dm_ioctl *ioc, const char *extra,
> +			 size_t extra_size)
> +{
> +	size_t offset = ioc->data_start;

uint32_t, not size_t

> +	if (offset + offsetof(struct dm_target_deps, dev) >= offset &&
> +	    offset + offsetof(struct dm_target_deps, dev) <= extra_size) {
> +		uint32_t i;
> +		size_t space = (extra_size - (offset +
> +			offsetof(struct dm_target_deps, dev))) / sizeof(__u64);
> +		const struct dm_target_deps *s =
> +			(const struct dm_target_deps *)(extra + offset);
> +		if (s->count > space)
> +			goto misplaced;

count is __u32, so space should probably be uint32_t instead of size_t

> +misplaced:

it's not standard in strace, but personal preference is to put a
single space before labels to help out with diff context

> +	size_t offset = ioc->data_start;

uint32_t

> +misplaced:

add a space

looks like many of these comments apply to the rest of this file,
so i'll refrain from repeating myself
-mike