[PATCH 3/3] Add btrfs ioctl support.

Dmitry V. Levin ldv at altlinux.org
Thu Mar 31 02:50:14 UTC 2016


On Wed, Mar 30, 2016 at 09:19:27PM -0400, Jeff Mahoney wrote:
[...]
> +#include "defs.h"
> +#include <linux/fs.h>
> +#include <linux/btrfs.h>

linux/btrfs.h was introduced by linux kernel commit v3.9-rc1~4^2~30^2~53,
while strace supports build with much older kernel headers than v3.9-rc1,
so please add a check.  I suppose most of the file heavily relies on
constants defined in linux/btrfs.h; if wrapping all of them with ifdefs
doesn't worth the effort, just provide a stub btrfs_ioctl for the case
of missing linux/btrfs.h and that would be enough.

> +#ifndef HAVE_BTRFS_IOCTL_FEATURE_FLAGS
> +struct btrfs_ioctl_feature_flags {
> +	__u64 compat_flags;
> +	__u64 compat_ro_flags;
> +	__u64 incompat_flags;
> +};
> +#endif
> +
> +#ifndef HAVE_BTRFS_IOCTL_DEFRAG_RANGE_ARGS
> +struct btrfs_ioctl_defrag_range_args {
> +	__u64 start;
> +	__u64 len;
> +	__u64 flags;
> +	__u32 extent_thresh;
> +	__u32 compress_type;
> +	__u32 unused[4];
> +};
> +#endif

Please use stdint types instead of __u32 and __u64.

> +#ifndef BTRFS_LABEL_SIZE
> +#define BTRFS_LABEL_SIZE 256
> +#endif
> +
> +#ifndef BTRFS_FIRST_FREE_OBJECTID
> +#define BTRFS_FIRST_FREE_OBJECTID 256ULL
> +#endif
> +
> +#ifndef BTRFS_IOC_GET_FEATURES
> +#define BTRFS_IOC_GET_FEATURES _IOR(BTRFS_IOCTL_MAGIC, 57, \
> +                                   struct btrfs_ioctl_feature_flags)
> +#define BTRFS_IOC_SET_FEATURES _IOW(BTRFS_IOCTL_MAGIC, 57, \
> +                                   struct btrfs_ioctl_feature_flags[2])
> +#define BTRFS_IOC_GET_SUPPORTED_FEATURES _IOR(BTRFS_IOCTL_MAGIC, 57, \
> +                                   struct btrfs_ioctl_feature_flags[3])
> +#endif
> +
> +#include "xlat/btrfs_balance_flags.h"
> +#include "xlat/btrfs_defrag_flags.h"
> +#include "xlat/btrfs_dev_replace_args.h"
> +#include "xlat/btrfs_dev_replace_cmds.h"
> +#include "xlat/btrfs_dev_replace_results.h"
> +#include "xlat/btrfs_dev_replace_state.h"
> +#include "xlat/btrfs_dev_stats_flags.h"
> +#include "xlat/btrfs_dev_stats_values.h"
> +#include "xlat/btrfs_features_compat.h"
> +#include "xlat/btrfs_features_compat_ro.h"
> +#include "xlat/btrfs_features_incompat.h"
> +#include "xlat/btrfs_key_types.h"
> +#include "xlat/btrfs_qgroup_ctl_cmds.h"
> +#include "xlat/btrfs_qgroup_inherit_flags.h"
> +#include "xlat/btrfs_qgroup_limit_flags.h"
> +#include "xlat/btrfs_qgroup_status_flags.h"
> +#include "xlat/btrfs_scrub_flags.h"
> +#include "xlat/btrfs_snap_flags_v2.h"
> +#include "xlat/btrfs_space_info_flags.h"
> +#include "xlat/btrfs_tree_objectids.h"
> +
> +static inline char
> +prnibble(char v)
> +{
> +	if (v >= 10)
> +		return 'a' + (v - 10);
> +	return '0' + v;
> +}
> +
> +/* Formats uuid, returns 0 if it's all zeroes */
> +static int
> +btrfs_unparse_uuid(unsigned char *uuid, char *out)
> +{
> +	int i;
> +	int ret = 0;
> +	for (i = 0; i < BTRFS_UUID_SIZE; i++) {
> +		if (i == 4 || i == 6 || i == 8 || i == 10)
> +			*out++ = '-';
> +		*out++ = prnibble(uuid[i] >> 4);
> +		*out++ = prnibble(uuid[i] & 0xf);
> +		if (uuid[i])
> +			ret = 1;
> +	}
> +	out[36] = '\0';

Why 36?  Is it by any chance BTRFS_UUID_SIZE * 2 + 4?

> +	return ret;
> +}
> +
> +static void
> +print_u64(const char *name, uint64_t value)
> +{
> +	tprintf(", %s=%" PRIu64, name, value);
> +}
> +
> +#define print_member_u64(obj, name) print_u64(#name, obj->name)
> +
> +static void
> +btrfs_print_balance_args(const char *name, struct btrfs_balance_args *bba)
> +{
> +	tprintf(", %s={profiles=%" PRI__u64, name, bba->profiles);
> +	print_member_u64(bba, usage);
> +	print_member_u64(bba, devid);
> +	print_member_u64(bba, pstart);
> +	print_member_u64(bba, pend);
> +	print_member_u64(bba, vstart);
> +	print_member_u64(bba, vend);
> +	print_member_u64(bba, target);
> +	printflags(btrfs_balance_flags, bba->flags, NULL);
> +	tprints("}");
> +}
> +
> +static void
> +btrfs_print_balance(struct tcb *tcp, const long arg, bool out)
> +{
> +	struct btrfs_ioctl_balance_args balance_args;
> +
> +	if (umove_or_printaddr(tcp, arg, &balance_args))
> +		return;
> +
> +	if (syserror(tcp))
> +		return;

umove_or_printaddr has already checked for syserror.

> +
> +	tprints("{flags=");
> +	printflags(btrfs_balance_flags, balance_args.flags, NULL);
> +	if (out)
> +		tprintf(", state=%" PRI__u64, balance_args.state);
> +
> +	if (balance_args.flags & BTRFS_BALANCE_DATA) {
> +		tprints(", ");
> +		btrfs_print_balance_args("data", &balance_args.data);
> +	}
> +	if (balance_args.flags & BTRFS_BALANCE_METADATA) {
> +		tprints(", ");
> +		btrfs_print_balance_args("meta", &balance_args.meta);
> +	}
> +	if (balance_args.flags & BTRFS_BALANCE_SYSTEM) {
> +		tprints(", ");
> +		btrfs_print_balance_args("sys", &balance_args.sys);
> +	}
> +	tprints("}");
> +}
> +
> +static void
> +btrfs_print_features(struct btrfs_ioctl_feature_flags *flags)
> +{
> +	tprints("{compat_flags=");
> +	printflags(btrfs_features_compat, flags->compat_flags,
> +		   "BTRFS_FEATURE_COMPAT_???");
> +
> +	tprints(", compat_ro_flags=");
> +	printflags(btrfs_features_compat_ro, flags->compat_ro_flags,
> +		   "BTRFS_FEATURE_COMPAT_RO_???");
> +
> +	tprints(", incompat_flags=");
> +	printflags(btrfs_features_incompat, flags->incompat_flags,
> +		   "BTRFS_FEATURE_INCOMPAT_???");
> +	tprints("}");
> +}
> +
> +static void
> +btrfs_print_qgroup_limit(struct btrfs_qgroup_limit *lim)
> +{
> +	tprints("{flags=");
> +	printflags(btrfs_qgroup_limit_flags, lim->flags, NULL);
> +	tprintf(", max_rfer=%" PRI__u64 ", max_excl=%" PRI__u64
> +		", rsv_rfer=%" PRI__u64 ", rsv_excl=%" PRI__u64 "}",
> +		lim->max_rfer, lim->max_excl,
> +		lim->rsv_rfer, lim->rsv_excl);
> +}
> +
> +static void
> +btrfs_print_key_type(uint32_t type)
> +{
> +	const char *str = xlookup(btrfs_key_types, type);
> +	tprintf("%u", type);
> +	if (str)
> +		tprintf(" /* %s */", str);
> +}
> +
> +static void
> +btrfs_print_objectid(uint64_t objectid)
> +{
> +	const char *str = xlookup(btrfs_tree_objectids, objectid);
> +	tprintf("%" PRIu64, objectid);
> +	if (str)
> +		tprintf(" /* %s */", str);
> +}
> +
> +int
> +btrfs_ioctl(struct tcb *tcp, const unsigned int code, const long arg)
> +{
> +	int ret = 0;
> +
> +	switch (code) {
> +	/* Take no arguments; Command only. */
> +	case BTRFS_IOC_TRANS_START:
> +	case BTRFS_IOC_TRANS_END:
> +	case BTRFS_IOC_SYNC:
> +	case BTRFS_IOC_SCRUB_CANCEL:
> +	case BTRFS_IOC_QUOTA_RESCAN_WAIT:
> +		break;
> +
> +	/* take a signed int */
> +	case BTRFS_IOC_CLONE: /* also FICLONE */
> +	case BTRFS_IOC_BALANCE_CTL:
> +		printnum_int(tcp, arg, "%d");

A comma is missing.

> +		break;
> +
> +	/* returns a 64 */
> +	case BTRFS_IOC_START_SYNC: /* R */
> +		if (entering(tcp))
> +			return 0;
> +	/* fallthrough */
> +	/* take a u64 */
> +	case BTRFS_IOC_DEFAULT_SUBVOL: /* W */
> +	case BTRFS_IOC_WAIT_SYNC: /* W */
> +		tprints(", ");
> +		printnum_int64(tcp, arg, "%" PRI__u64);

You can safely use PRIu64 here.

> +		break;
> +
> +	/* u64 but describe a flags bitfield; We can make that symbolic */
> +	case BTRFS_IOC_SUBVOL_GETFLAGS: { /* R */
> +		uint64_t flags;
> +		if (entering(tcp))
> +			return 0;
> +		if (syserror(tcp))
> +			break;

You has to print something before "break".

> +		tprints(", ");
> +		if (umove_or_printaddr(tcp, arg, &flags))
> +			break;

The check for syserror is not needed here as umove_or_printaddr does
the right thing.

> +		printflags(btrfs_snap_flags_v2, flags, "BTRFS_SUBVOL_???");
> +		break;
> +	}
> +	case BTRFS_IOC_SUBVOL_SETFLAGS: { /* W */
> +		uint64_t flags;
> +		if (exiting(tcp))
> +			break;

The check for exiting is redundant because of "break" -> RVAL_DECODED | 1.

> +		tprints(", ");
> +		if (umove_or_printaddr(tcp, arg, &flags))
> +			break;
> +		printflags(btrfs_snap_flags_v2, flags, "BTRFS_SUBVOL_???");
> +		break;
> +	}
> +
> +	/* More complex types */
> +	case BTRFS_IOC_BALANCE_V2: /* RW */
> +		if (entering(tcp)) {
> +			tprints(", ");
> +			btrfs_print_balance(tcp, arg, false);
> +			return 0;
> +		}
> +		if (exiting(tcp)) {

It's either entering or exiting, so you can use

	if (entering(tcp)) {
		...
	} else {
		...
	}

> +			if (syserror(tcp))
> +				break;
> +			tprints(" => ");
> +			btrfs_print_balance(tcp, arg, true);
> +		}
> +		break;
> +	case BTRFS_IOC_BALANCE_PROGRESS: /* R */
> +		if (exiting(tcp)) {
> +			tprints(", ");
> +			btrfs_print_balance(tcp, arg, true);
> +		}
> +		break;

It has to be

	if (entering(tcp))
		return 0;
	tprints(", ");
	btrfs_print_balance(tcp, arg, true);
	break;

> +	case BTRFS_IOC_CLONE_RANGE: /* W */
> +		if (entering(tcp)) {
> +			struct btrfs_ioctl_clone_range_args args;
> +
> +			tprints(", ");
> +			if (umove_or_printaddr(tcp, arg, &args))
> +				break;
> +			tprintf("{src_fd=%" PRI__s64 ", "
> +				"src_offset=%" PRI__u64 ", "
> +				"src_length=%" PRI__u64 ", "
> +				"dest_offset=%" PRI__u64 "}",
> +				args.src_fd, args.src_offset,
> +				args.src_length, args.dest_offset);
> +		}
> +		break;

As it always does a "break", it's never in an exiting state,
so the check for entering is redundant.

> +
> +	case BTRFS_IOC_DEFRAG_RANGE: /* W */
> +		if (entering(tcp)) {
> +			struct btrfs_ioctl_defrag_range_args args;
> +
> +			tprints(", ");
> +			if (umove_or_printaddr(tcp, arg, &args))
> +				break;
> +			tprintf("{start=%" PRI__u64 ", len=%" PRI__u64 ", "
> +				"flags=", args.start, args.len);
> +			printflags(btrfs_defrag_flags, args.flags, NULL);
> +			tprintf(", extent_thresh=%u, compress_type=%u}",
> +				args.extent_thresh, args.compress_type);
> +		}
> +		break;

Likewise.

At this point I'm giving up reviewing btrfs.c,
please correct these issues.

> --- a/configure.ac
> +++ b/configure.ac
> @@ -433,6 +433,21 @@ AC_CHECK_HEADERS([linux/bpf.h], [
>  	fi
>  ])
>  
> +AC_CHECK_HEADERS([linux/btrfs.h], [
> +	AC_CHECK_MEMBERS([struct btrfs_ioctl_feature_flags.compat_flags],
> +			 AC_DEFINE(HAVE_BTRFS_IOCTL_FEATURE_FLAGS, [1],
> +				[Define 1 to if struct strfs_ioctl_feature_flags exists]),,
> +			 [#include <linux/btrfs.h>])
> +	AC_CHECK_MEMBERS([struct btrfs_ioctl_fs_info_args.nodesize],
> +			AC_DEFINE(HAVE_BTRFS_IOCTL_FS_INFO_ARGS_NODESIZE, [1],
> +				[Define to 1 if struct btrfs_ioctl_fs_info_args.nodesize exists]),,
> +			 [#include <linux/btrfs.h>])
> +	AC_CHECK_MEMBERS([struct btrfs_ioctl_defrag_range_args.start],
> +			AC_DEFINE(HAVE_BTRFS_IOCTL_DEFRAG_RANGE_ARGS, [1],
> +				[Define to 1 if struct btrfs_ioctl_defrag_range_args exists]),,
> +			 [#include <linux/btrfs.h>])
> +])

Do you really need all these AC_DEFINEs?  There must be a simpler way, e.g.

AC_CHECK_HEADERS([linux/btrfs.h], [
        AC_CHECK_MEMBERS(m4_normalize([
		struct btrfs_ioctl_feature_flags.compat_flags,
		struct btrfs_ioctl_fs_info_args.nodesize,
		struct btrfs_ioctl_defrag_range_args.start
			]),,, [#include <linux/btrfs.h>])
])


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


More information about the Strace-devel mailing list