Hi Goffredo,

Thanks for your review.

On 12/19/2012 02:00 AM, Goffredo Baroncelli wrote:

Hi Jeff,

On 12/18/2012 04:31 AM, Miao Xie wrote:
[...]

quoted

quoted

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c

[...]

quoted

quoted

+static int btrfs_ioctl_set_fslabel(struct file *file, void __user *arg)
+{
+	struct btrfs_root *root = BTRFS_I(fdentry(file)->d_inode)->root;
+	struct btrfs_super_block *super_block = root->fs_info->super_copy;
+	struct btrfs_trans_handle *trans;
+	char label[BTRFS_LABEL_SIZE];
+	int ret;
+
+	if (!capable(CAP_SYS_ADMIN))
+		return -EPERM;
+
+	if (copy_from_user(label, arg, sizeof(label)))
+		return -EFAULT;
+
+	if (strnlen(label, BTRFS_LABEL_SIZE) == BTRFS_LABEL_SIZE)
+		return -EINVAL;
+
+	ret = mnt_want_write_file(file);
+	if (ret)
+		return ret;
+
+	mutex_lock(&root->fs_info->volume_mutex);
+	trans = btrfs_start_transaction(root, 1);
+	if (IS_ERR(trans)) {
+		ret = PTR_ERR(trans);
+		goto out_unlock;
+	}
+
+	strcpy(super_block->label, label);

I think that you removed for mistake the following line

+	label[BTRFS_LABEL_SIZE - 1] = '\0';

I removed it since it was used to cut the label string off the max array
size but now we have the previous strnlen().

In the V5 patch it was present.

May be we could replace strcpy() with strlcpy(super_block->label, label,
BTRFS_LABEL_SIZE-1) ?

That is ok to me. However, it should be strlcpy(super_block->label,
label, BTRFS_LABEL_SIZE) ranther than 'BTRFS_LABREL_SIZE -1' because
strlcpy() does "size - 1" internally. i.e.

strlcpy(char *d, const char *s, size_t size)
{	
	size_t ret = strlen(s);

	.....
	size_t len = (ret >= size) ? size - 1 : ret;
	....
}

But does the current implementation make anything wrong? :)

Thanks,
-Jeff

BR
G.Baroncelli