[PATCH 3/3] Add btrfs ioctl support.

Jeff Mahoney jeffm at suse.com
Thu Mar 31 04:08:41 UTC 2016


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

Again, thanks for the review.

> 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; 

Yeah, it definitely does.

> 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.

Yeah, I think that's sane.

>> +#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.

Fixed.

>> +#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?

Yes. Fixed.

>> +	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 *bb=
> a)
>> +{
>> +	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.

Ok.

>> +
>> +	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.

Fixed.

>> +		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.

Fixed.

>> +		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".

Fixed.

>> +		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.

Fixed.

>> +		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.

Fixed.

>> +		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 {
> 		...
> 	}

Fixed.

>> +			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;

Fixed.

>> +	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.

Fixed.

>> +
>> +	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.

Fair enough.  You've given enough examples of mistakes that I can fix
the rest of them without itemization.  Thanks for the effort.

>> --- a/configure.ac
>> +++ b/configure.ac
>> @@ -433,6 +433,21 @@ AC_CHECK_HEADERS([linux/bpf.h], [
>>  	fi
>>  ])
>> =20
>> +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>])
> ])

Yeah, that'll work.

-Jeff

-- 
Jeff Mahoney
SUSE Labs

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 827 bytes
Desc: OpenPGP digital signature
URL: <http://lists.strace.io/pipermail/strace-devel/attachments/20160331/e3105b5b/attachment.bin>


More information about the Strace-devel mailing list