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@xxxxxxxxxxx> 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);
> }
>
> >
> > ...
> >
>
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
