LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Dave Hansen <dave@linux.vnet.ibm.com>
To: Michael Halcrow <mhalcrow@us.ibm.com>
Cc: Andrew Morton <akpm@linux-foundation.org>,
	LKML <linux-kernel@vger.kernel.org>,
	Dustin Kirkland <dustin.kirkland@gmail.com>,
	Eric Sandeen <sandeen@redhat.com>,
	Tyler Hicks <tchicks@us.ibm.com>,
	David Kleikamp <shaggy@us.ibm.com>
Subject: Re: [PATCH 3/5] eCryptfs: Filename Encryption: Encoding and encryption functions
Date: Wed, 05 Nov 2008 10:17:36 -0800	[thread overview]
Message-ID: <1225909056.12673.632.camel@nimitz> (raw)
In-Reply-To: <20081104214120.GC6677@halcrowt61p.austin.ibm.com>

On Tue, 2008-11-04 at 15:41 -0600, Michael Halcrow wrote:
> +static int ecryptfs_copy_filename(char **copied_name, size_t *copied_name_size,
> +                                 const char *name, size_t name_size)
> +{
> +       int rc = 0;
> +
> +       (*copied_name) = kmalloc((name_size + 2), GFP_KERNEL);
> +       if (!(*copied_name)) {
> +               rc = -ENOMEM;
> +               goto out;
> +       }
> +       memcpy((void *)(*copied_name), (void *)name, name_size);
> +       (*copied_name)[(name_size)] = '\0';     /* Only for convenience
> +                                                * in printing out the
> +                                                * string in debug
> +                                                * messages */
> +       (*copied_name_size) = (name_size + 1);
> +out:
> +       return rc;
> +}

Might this be a good place to use ERR_PTR()?  The pointer arithmetic
here looks a bit goofy.  Is this all just to get 'copied_name_size'
returned?  Why does it even matter if it is only there for printk'ing?

But, as I look at it, you do this all over the patch(es) and I think it
really reduces the readability everywhere.

If you truly need to track the actual allocated name size, I'd suggest
just having a:

struct e...c_filename {
	char *name;
	char *len;
};

Stack allocate that sucker just like you stack allocate
'copied_name_size', and then pass *it* around.

	filename->name = foo;
	filename->bar = 1234;

is way more readable than:

+                       (*encoded_name) = NULL;
+                       (*encoded_name_size) = 0;

and

+                       (*encoded_name)[(*encoded_name_size)] = '\0';
+                       (*encoded_name_size)++;

I'm certainly guilty of overly-verbose names for things, but I think:

	ECRYPTFS_FNEK_ENCRYPTED_FILENAME_PREFIX_SIZE

may be a few characters too long.  :)

This construct:

+               decoded_name = kmalloc(decoded_name_size, GFP_KERNEL);
+               if (!decoded_name) {
+                       printk(KERN_ERR "%s: Out of memory whilst attempting "
+                              "to kmalloc [%Zd] bytes\n", __func__,
+                              decoded_name_size);
+                       rc = -ENOMEM;
+                       goto out;
+               }

also appears all over.  Personally, I think this is way too verbose, but
I can also see how it would be useful.  But, the crappy part is that
there is nothing unique in each of this printk()s to help the dmesg
reader to figure out what is going on.

I think you'd save yourself a lot of code if you had an
ecryptfs_kmalloc(), did the NULL check and printk() in there, and also
added a stack dump.

>         rc = write_tag_66_packet(auth_tok->token.private_key.signature,
> -                                ecryptfs_code_for_cipher_string(crypt_stat),
> +                                ecryptfs_code_for_cipher_string(
> +                                        crypt_stat->cipher,
> +                                        crypt_stat->key_size),
>                                  crypt_stat, &payload, &payload_len);
>         if (rc) {

Why not just have ecryptfs_code_for_cipher_string() do the ->cipher and
->key_size lookup?  

-- Dave


  reply	other threads:[~2008-11-05 18:18 UTC|newest]

Thread overview: 20+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2008-11-04 21:37 [PATCH 0/5] eCryptfs: Filename Encryption Michael Halcrow
2008-11-04 21:39 ` [PATCH 1/5] eCryptfs: Filename Encryption: Tag 70 packets Michael Halcrow
2008-11-06 22:12   ` Andrew Morton
2008-11-12 17:01     ` [PATCH] eCryptfs: Replace %Z with %z Michael Halcrow
2008-11-12 17:04     ` [PATCH] eCryptfs: Fix data types (int/size_t) Michael Halcrow
2008-11-12 17:06     ` [PATCH] eCryptfs: kerneldoc for ecryptfs_parse_tag_70_packet() Michael Halcrow
2008-11-04 21:39 ` [PATCH 2/5] eCryptfs: Filename Encryption: Header updates Michael Halcrow
2008-11-04 21:41 ` [PATCH 3/5] eCryptfs: Filename Encryption: Encoding and encryption functions Michael Halcrow
2008-11-05 18:17   ` Dave Hansen [this message]
2008-11-06 21:01     ` Michael Halcrow
2008-11-06 22:12   ` Andrew Morton
2008-11-12 17:11     ` [PATCH] eCryptfs: Clean up ecryptfs_decode_from_filename() Michael Halcrow
2008-11-04 21:42 ` [PATCH 4/5] eCryptfs: Filename Encryption: filldir, lookup, and readlink Michael Halcrow
2008-11-04 21:43 ` [PATCH 5/5] eCryptfs: Filename Encryption: mount option Michael Halcrow
2008-11-06 22:13   ` Andrew Morton
2008-11-14 16:47     ` Michael Halcrow
2008-11-05 15:57 ` [PATCH 0/5] eCryptfs: Filename Encryption Pavel Machek
2008-11-06 20:27   ` Michael Halcrow
2008-11-06 20:52     ` Dave Kleikamp
2008-11-06 22:11       ` Michael Halcrow

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1225909056.12673.632.camel@nimitz \
    --to=dave@linux.vnet.ibm.com \
    --cc=akpm@linux-foundation.org \
    --cc=dustin.kirkland@gmail.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mhalcrow@us.ibm.com \
    --cc=sandeen@redhat.com \
    --cc=shaggy@us.ibm.com \
    --cc=tchicks@us.ibm.com \
    --subject='Re: [PATCH 3/5] eCryptfs: Filename Encryption: Encoding and encryption functions' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).