From mboxrd@z Thu Jan  1 00:00:00 1970
From: Andrew Morton <akpm@linux-foundation.org>
Subject: Re: [PATCH v2 2/4] hfsplus: add functionality of manipulating by
 records in attributes tree
Date: Mon, 24 Sep 2012 15:49:13 -0700
Message-ID: <20120924154913.0afbc1e3.akpm@linux-foundation.org>
References: <B12F9C3E-4B9C-436D-B4BF-965C377B93E7@dubeyko.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Cc: linux-fsdevel@vger.kernel.org,
	Christoph Hellwig <hch@infradead.org>,
	Al Viro <viro@zeniv.linux.org.uk>,
	Hin-Tak Leung <htl10@users.sourceforge.net>
To: Vyacheslav Dubeyko <slava@dubeyko.com>
Return-path: <linux-fsdevel-owner@vger.kernel.org>
Received: from mail.linuxfoundation.org ([140.211.169.12]:35821 "EHLO
	mail.linuxfoundation.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
	with ESMTP id S1751494Ab2IXWtO (ORCPT
	<rfc822;linux-fsdevel@vger.kernel.org>);
	Mon, 24 Sep 2012 18:49:14 -0400
In-Reply-To: <B12F9C3E-4B9C-436D-B4BF-965C377B93E7@dubeyko.com>
Sender: linux-fsdevel-owner@vger.kernel.org
List-ID: <linux-fsdevel.vger.kernel.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);
}

>
> ...
>