linux-kernel - Re: [PATCH v9 2/3] fs: fat: Add volume label entry method function

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 7 Feb 2018 18:20:25 +0100
From:   Pali Rohár <pali.rohar@...il.com>
To:     Chen Guanqiao <chen.chenchacha@...mail.com>
Cc:     hirofumi@...l.parknet.co.jp, linux-kernel@...r.kernel.org,
        andy.shevchenko@...il.com
Subject: Re: [PATCH v9 2/3] fs: fat: Add volume label entry method function

On Thursday 08 February 2018 00:14:06 Chen Guanqiao wrote:
> Signed-off-by: Chen Guanqiao <chen.chenchacha@...mail.com>

Missing commit message and information what are the new proposed
functions suppose to do.

> ---
>  fs/fat/dir.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 47 insertions(+)
> 
> diff --git a/fs/fat/dir.c b/fs/fat/dir.c
> index 8e100c3bf72c..d5286402c829 100644
> --- a/fs/fat/dir.c
> +++ b/fs/fat/dir.c
> @@ -881,6 +881,53 @@ static int fat_get_short_entry(struct inode *dir, loff_t *pos,
> 	return -ENOENT;
>  }
> 
> +int fat_get_volume_label_entry(struct inode *dir, struct buffer_head **bh,
> +			       struct msdos_dir_entry **de)
> +{
> +	loff_t pos = 0;
> +
> +	*bh = NULL;
> +	*de = NULL;
> +	while (fat_get_entry(dir, &pos, bh, de) >= 0) {
> +		if (((*de)->attr & ATTR_VOLUME) && ((*de)->attr != ATTR_EXT) &&
> +		    !IS_FREE((*de)->name))
> +			return 0;
> +	}
> +	return -ENOENT;
> +}
> +EXPORT_SYMBOL_GPL(fat_get_volume_label_entry);

Why both functions are exported? In this patch series are used only in
file.c which is in the same object file.

> +
> +int fat_add_volume_label_entry(struct inode *dir, const unsigned char *name,
> +			       struct timespec *ts)
> +{
> +	struct msdos_sb_info *sbi = MSDOS_SB(dir->i_sb);
> +	struct msdos_dir_entry de;
> +	struct fat_slot_info sinfo;
> +	__le16 time, date;
> +	int err;
> +
> +	memcpy(de.name, name, MSDOS_NAME);
> +	de.attr = ATTR_VOLUME;
> +	de.lcase = 0;
> +	fat_time_unix2fat(sbi, ts, &time, &date, NULL);
> +	de.cdate = de.adate = 0;
> +	de.ctime = 0;
> +	de.ctime_cs = 0;
> +	de.time = time;
> +	de.date = date;
> +	fat_set_start(&de, 0);
> +	de.size = 0;
> +
> +	err = fat_add_entries(dir, &de, 1, &sinfo);
> +	if (err)
> +		return err;
> +
> +	brelse(sinfo.bh);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(fat_add_volume_label_entry);

This function looks like copy+paste of function msdos_add_entry().
Please de-duplicate code as this would lead to problems in future.

Also who is supposed to format dos name? Caller or callee? There is no
information neither in commit message nor in comments.

> +
>  /*
>   * The ".." entry can not provide the "struct fat_slot_info" information
>   * for inode, nor a usable i_pos. So, this function provides some information
> --
> 2.14.3

-- 
Pali Rohár
pali.rohar@...il.com

Download attachment "signature.asc" of type "application/pgp-signature" (196 bytes)