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

[Vyacheslav Dubeyko <slava@dubeyko.com>]

Hi,

Ok. I agree with all your remarks. I will correct it.

Do I need prepare a new version of patch set or simply patch for these
corrections?

With the best regards,
Vyacheslav Dubeyko.

On Mon, 2012-09-24 at 15:49 -0700, Andrew Morton wrote:
> 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);
> }
> 
> >
> > ...
> >
>