[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