Hi, add a missed a comment.
> -----Original Message-----
> From: Misono Tomohiro [mailto:misono.tomoh...@jp.fujitsu.com]
> Sent: Tuesday, May 15, 2018 3:04 PM
> To: Gu, Jinxiang/顾 金香 <g...@cn.fujitsu.com>; linux-btrfs@vger.kernel.org
> Subject: Re: [PATCH v4 1/3] btrfs: Add unprivileged ioctl which returns
> subvolume information
>
> On 2018/05/15 15:31, Gu, Jinxiang/顾 金香 wrote:
> > Hi,
> >
> >> -----Original Message-----
> >> From: linux-btrfs-ow...@vger.kernel.org
> >> [mailto:linux-btrfs-ow...@vger.kernel.org] On Behalf Of Tomohiro
> >> Misono
> >> Sent: Friday, May 11, 2018 3:26 PM
> >> To: linux-btrfs@vger.kernel.org
> >> Subject: [PATCH v4 1/3] btrfs: Add unprivileged ioctl which returns
> >> subvolume information
> >>
> >> Add new unprivileged ioctl BTRFS_IOC_GET_SUBVOL_INFO which returns the
> >> information of subvolume containing this inode.
> >> (i.e. returns the information in ROOT_ITEM and ROOT_BACKREF.)
> >>
> >> Signed-off-by: Tomohiro Misono <misono.tomoh...@jp.fujitsu.com>
> >> ---
> >> fs/btrfs/ioctl.c | 129
> >> +++++++++++++++++++++++++++++++++++++++++++++
> >> include/uapi/linux/btrfs.h | 51 ++++++++++++++++++
> >> 2 files changed, 180 insertions(+)
> >>
> >> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c index
> >> 48e2ddff32bd..64b23e22852f 100644
> >> --- a/fs/btrfs/ioctl.c
> >> +++ b/fs/btrfs/ioctl.c
> >> @@ -2242,6 +2242,133 @@ static noinline int btrfs_ioctl_ino_lookup(struct
> >> file *file,
> >> return ret;
> >> }
> >>
> >> +/* Get the subvolume information in BTRFS_ROOT_ITEM and
> >> +BTRFS_ROOT_BACKREF */ static noinline int
> >> btrfs_ioctl_get_subvol_info(struct file *file,
> >> + void __user *argp)
> >> +{
> >> + struct btrfs_ioctl_get_subvol_info_args *subvol_info;
> >> + struct btrfs_root *root;
> >> + struct btrfs_path *path;
> >> + struct btrfs_key key;
> >> +
> >> + struct btrfs_root_item root_item;
> >> + struct btrfs_root_ref *rref;
> >> + struct extent_buffer *l;
> >> + int slot;
> >> +
> >> + unsigned long item_off;
> >> + unsigned long item_len;
> >> +
> >> + struct inode *inode;
> >> + int ret;
> >> +
> >> + path = btrfs_alloc_path();
> >> + if (!path)
> >> + return -ENOMEM;
> >> +
> >> + subvol_info = kzalloc(sizeof(*subvol_info), GFP_KERNEL);
> >> + if (!subvol_info) {
> >> + btrfs_free_path(path);
> >> + return -ENOMEM;
> >> + }
> >> + inode = file_inode(file);
> >> +
> >> + root = BTRFS_I(inode)->root->fs_info->tree_root;
> >> + key.objectid = BTRFS_I(inode)->root->root_key.objectid;
> >> + key.type = BTRFS_ROOT_ITEM_KEY;
> >> + key.offset = 0;
> >> + ret = btrfs_search_slot(NULL, root, &key, path, 0, 0);
> >> + if (ret < 0) {
> >> + goto out;
> >> + } else if (ret > 0) {
> >> + u64 objectid = key.objectid;
> >> +
> >> + if (path->slots[0] >= btrfs_header_nritems(path->nodes[0])) {
> >> + ret = btrfs_next_leaf(root, path);
> >> + if (ret < 0)
> >> + return ret;
> > Should goto out; to free subvol_info and path.
> Thanks, will update both.
>
Since btrfs_next_leaf may return 1 when nritems of next leaf is 0,
So, btrfs_item_key_to_cpu(path->nodes[0], &key, path->slots[0]); may goes wrong.
And I think it should add a judge before btrfs_item_key_to_cpu.
> >> + }
> >> +
> >> + /* If the subvolume is a snapshot, offset is not zero */
> >> + btrfs_item_key_to_cpu(path->nodes[0], &key, path->slots[0]);
> >> + if (key.objectid != objectid ||
> >> + key.type != BTRFS_ROOT_ITEM_KEY) {
> >> + ret = -ENOENT;
> >> + goto out;
> >> + }
> >> + }
> >> +
> >> + l = path->nodes[0];
> >> + slot = path->slots[0];
> >> + item_off = btrfs_item_ptr_offset(l, slot);
> >> + item_len = btrfs_item_size_nr(l, slot);
> >> + read_extent_buffer(l, &root_item, item_off, item_len);
> >> +
> >> + subvol_info->id = key.objectid;
> >> +
> >> + subvol_info->generation = btrfs_root_generation(&root_item);
> >> + subvol_info->flags = btrfs_root_flags(&root_item);
> >> +
> >> + memcpy(subvol_info->uuid, root_item.uuid, BTRFS_UUID_SIZE);
> >> + memcpy(subvol_info->parent_uuid, root_item.parent_uuid,
> >> + BTRFS_UUID_SIZE);
> >> + memcpy(subvol_info->received_uuid, root_item.received_uuid,
> >> + BTRFS_UUID_SIZE);
> >> +
> >> + subvol_info->ctransid = btrfs_root_ctransid(&root_item);
> >> + subvol_info->ctime.sec = btrfs_stack_timespec_sec(&root_item.ctime);
> >> + subvol_info->ctime.nsec =
> >> +btrfs_stack_timespec_nsec(&root_item.ctime);
> >> +
> >> + subvol_info->otransid = btrfs_root_otransid(&root_item);
> >> + subvol_info->otime.sec = btrfs_stack_timespec_sec(&root_item.otime);
> >> + subvol_info->otime.nsec =
> >> +btrfs_stack_timespec_nsec(&root_item.otime);
> >> +
> >> + subvol_info->stransid = btrfs_root_stransid(&root_item);
> >> + subvol_info->stime.sec = btrfs_stack_timespec_sec(&root_item.stime);
> >> + subvol_info->stime.nsec =
> >> +btrfs_stack_timespec_nsec(&root_item.stime);
> >> +
> >> + subvol_info->rtransid = btrfs_root_rtransid(&root_item);
> >> + subvol_info->rtime.sec = btrfs_stack_timespec_sec(&root_item.rtime);
> >> + subvol_info->rtime.nsec =
> >> +btrfs_stack_timespec_nsec(&root_item.rtime);
> >> +
> >> + btrfs_release_path(path);
> >> + key.type = BTRFS_ROOT_BACKREF_KEY;
> >> + key.offset = 0;
> >> + ret = btrfs_search_slot(NULL, root, &key, path, 0, 0);
> >> + if (ret < 0) {
> >> + goto out;
> >> + } else if (path->slots[0] >= btrfs_header_nritems(path->nodes[0])) {
> >> + ret = btrfs_next_leaf(root, path);
> >> + if (ret < 0)
> >> + return ret;
> > Should goto out; to free subvol_info and path.
> >> + }
> >> +
> >> + l = path->nodes[0];
> >> + slot = path->slots[0];
> >> + btrfs_item_key_to_cpu(l, &key, slot);
> >> + if (key.objectid == subvol_info->id &&
> >> + key.type == BTRFS_ROOT_BACKREF_KEY){
> >> + subvol_info->parent_id = key.offset;
> >> +
> >> + rref = btrfs_item_ptr(l, slot, struct btrfs_root_ref);
> >> + subvol_info->dirid = btrfs_root_ref_dirid(l, rref);
> >> +
> >> + item_off = btrfs_item_ptr_offset(l, slot)
> >> + + sizeof(struct btrfs_root_ref);
> >> + item_len = btrfs_item_size_nr(l, slot)
> >> + - sizeof(struct btrfs_root_ref);
> >> + read_extent_buffer(l, subvol_info->name, item_off, item_len);
> >> + }
> > If this if is not correct(ex. corrupt filesystem without backref),
> > should it return -ENOENT, or its be ok without parent_id, dirid and name.
> > Suggest to add logic of else.
>
> If backref does not exist (except top-level subvolume), it means filesystem
> corruption.
> So, I'd like to return -EUCLEAN here.
>
> Thanks,
> Tomohiro Misono
>
> >
> >> +
> >> + if (copy_to_user(argp, subvol_info, sizeof(*subvol_info)))
> >> + ret = -EFAULT;
> >> +
> >> +out:
> >> + kzfree(subvol_info);
> >> + btrfs_free_path(path);
> >> + return ret;
> >> +}
> >> +
> >> static noinline int btrfs_ioctl_snap_destroy(struct file *file,
> >> void __user *arg)
> >> {
> >> @@ -5374,6 +5501,8 @@ long btrfs_ioctl(struct file *file, unsigned int
> >> return btrfs_ioctl_get_features(file, argp);
> >> case BTRFS_IOC_SET_FEATURES:
> >> return btrfs_ioctl_set_features(file, argp);
> >> + case BTRFS_IOC_GET_SUBVOL_INFO:
> >> + return btrfs_ioctl_get_subvol_info(file, argp);
> >> }
> >>
> >> return -ENOTTY;
> >> diff --git a/include/uapi/linux/btrfs.h b/include/uapi/linux/btrfs.h
> >> index c8d99b9ca550..ed053852c71f 100644
> >> --- a/include/uapi/linux/btrfs.h
> >> +++ b/include/uapi/linux/btrfs.h
> >> @@ -725,6 +725,55 @@ struct btrfs_ioctl_send_args {
> >> __u64 reserved[4]; /* in */
> >> };
> >>
> >> +struct btrfs_ioctl_get_subvol_info_args {
> >> + /* All filed is out */
> >> + /* Id of this subvolume */
> >> + __u64 id;
> >> + /* Name of this subvolume, used to get the real name at mount point */
> >> + char name[BTRFS_VOL_NAME_MAX + 1];
> >> + /*
> >> + * Id of the subvolume which contains this subvolume.
> >> + * Zero for top-level subvolume or deleted subvolume
> >> + */
> >> + __u64 parent_id;
> >> + /*
> >> + * Inode number of the directory which contains this subvolume.
> >> + * Zero for top-level subvolume or deleted subvolume
> >> + */
> >> + __u64 dirid;
> >> +
> >> + /* Latest transaction id of this subvolume */
> >> + __u64 generation;
> >> + /* Flags of this subvolume */
> >> + __u64 flags;
> >> +
> >> + /* uuid of this subvolume */
> >> + __u8 uuid[BTRFS_UUID_SIZE];
> >> + /*
> >> + * uuid of the subvolume of which this subvolume is a snapshot.
> >> + * All zero for non-snapshot subvolume
> >> + */
> >> + __u8 parent_uuid[BTRFS_UUID_SIZE];
> >> + /*
> >> + * uuid of the subvolume from which this subvolume is received.
> >> + * All zero for non-received subvolume
> >> + */
> >> + __u8 received_uuid[BTRFS_UUID_SIZE];
> >> +
> >> + /* Transaction id indicates when change/create/send/receive happens */
> >> + __u64 ctransid;
> >> + __u64 otransid;
> >> + __u64 stransid;
> >> + __u64 rtransid;
> >> + /* Time corresponds to c/o/s/rtransid */
> >> + struct btrfs_ioctl_timespec ctime;
> >> + struct btrfs_ioctl_timespec otime;
> >> + struct btrfs_ioctl_timespec stime;
> >> + struct btrfs_ioctl_timespec rtime;
> >> +
> >> + __u64 reserved[8];
> >> +};
> >> +
> >> /* Error codes as returned by the kernel */ enum btrfs_err_code {
> >> BTRFS_ERROR_DEV_RAID1_MIN_NOT_MET = 1, @@ -843,5 +892,7 @@ enum
> >> btrfs_err_code {
> >> struct btrfs_ioctl_vol_args_v2) #define
> >> BTRFS_IOC_LOGICAL_INO_V2 _IOWR(BTRFS_IOCTL_MAGIC, 59, \
> >> struct btrfs_ioctl_logical_ino_args)
> >> +#define BTRFS_IOC_GET_SUBVOL_INFO _IOR(BTRFS_IOCTL_MAGIC, 60, \
> >> + struct btrfs_ioctl_get_subvol_info_args)
> >>
> >> #endif /* _UAPI_LINUX_BTRFS_H */
> >> --
> >> 2.14.3
> >>
> >
> > Reviewed-by: Gu Jinxiang <g...@cn.fujitsu.com>
> >>
> >> --
> >> To unsubscribe from this list: send the line "unsubscribe
> >> linux-btrfs" in the body of a message to majord...@vger.kernel.org
> >> More majordomo info at http://vger.kernel.org/majordomo-info.html
> >>
> >
N�Р骒r��y����b�X�肚�v�^�)藓{.n�+�伐�{�n谶�)��骅w* jg�报 �����茛j/�赇z罐���2���ㄨ��&�)摺�a囤�� �G���h��j:+v���w��佶
