Re: [PATCH v2 2/4] hfsplus: add functionality of manipulating by records in attributes tree

[Andrew Morton <akpm@linux-foundation.org>]

On Sun, 23 Sep 2012 18:49:24 +0400
Vyacheslav Dubeyko <slava@dubeyko.com> wrote:

> This patch adds functionality of manipulating by records in attributes tree.

Some minor things:

>
> ...
>
> +int hfsplus_attr_build_key(struct super_block *sb, hfsplus_btree_key *key,
> +			u32 cnid, const char *name)
> +{
> +	int len;
> +
> +	memset(key, 0, sizeof(struct hfsplus_attr_key));
> +	key->attr.cnid = cpu_to_be32(cnid);
> +	if (name) {
> +		len = strlen(name);
> +		if (len > HFSPLUS_ATTR_MAX_STRLEN) {
> +			printk(KERN_ERR "hfs: invalid xattr name's length\n");
> +			return -EINVAL;
> +		}
> +		hfsplus_asc2uni(sb,
> +				(struct hfsplus_unistr *)&key->attr.key_name,
> +				HFSPLUS_ATTR_MAX_STRLEN, name, len);
> +		len = be16_to_cpu(key->attr.key_name.length);
> +	} else {
> +		key->attr.key_name.length = 0;
> +		len = 0;
> +	}
> +	/* The key_length (be16) doesn't summed in the lenght of whole key.

s/lenght/length/

Also, this sentence is rather hard to understand.  Rephrase, please?

> +	   But the length of the string (be16) should be included in sum.
> +	   So, offsetof(hfsplus_attr_key, key_name) is a trick that give
> +	   right value. */

The usual layout style for multiline comments is

	/*
	 * ...
	 * ...
	 */

> +	key->key_len =
> +		cpu_to_be16(offsetof(struct hfsplus_attr_key, key_name) +
> +				2 * len);
> +
> +	return 0;
> +}
> +
> +void hfsplus_attr_build_key_uni(hfsplus_btree_key *key,
> +					u32 cnid,
> +					struct hfsplus_attr_unistr *name)
> +{
> +	int ustrlen;
> +
> +	memset(key, 0, sizeof(struct hfsplus_attr_key));
> +	ustrlen = be16_to_cpu(name->length);
> +	key->attr.cnid = cpu_to_be32(cnid);
> +	key->attr.key_name.length = cpu_to_be16(ustrlen);
> +	ustrlen *= 2;
> +	memcpy(key->attr.key_name.unicode, name->unicode, ustrlen);
> +	/* The key_length (be16) doesn't summed in the lenght of whole key.
> +	   But the length of the string (be16) should be included in sum.
> +	   So, offsetof(hfsplus_attr_key, key_name) is a trick that give
> +	   right value. */

dittoes.

> +	key->key_len =
> +		cpu_to_be16(offsetof(struct hfsplus_attr_key, key_name) +
> +				ustrlen);
> +}
> +
> +hfsplus_attr_entry *hfsplus_alloc_attr_entry(void)
> +{
> +	hfsplus_attr_entry *entry;
> +	entry = kmem_cache_alloc(hfsplus_attr_tree_cachep, GFP_KERNEL);
> +
> +	return (entry) ? entry : NULL;
> +}

This is rather verbose.  It could be

hfsplus_attr_entry *hfsplus_alloc_attr_entry(void)
{
	return kmem_cache_alloc(hfsplus_attr_tree_cachep, GFP_KERNEL);
}

>
> ...
>