On Sun, Jan 29, 2012 at 5:39 PM, Jesper Juhl <[email protected]> wrote:
> We declare 'exact' without initializing it and then do:
>
> [...]
> ? ? ? ?if (strlen(p->cru_driver_name))
> ? ? ? ? ? ? ? ?exact = 1;
>
> ? ? ? ?if (priority && !exact)
> ? ? ? ? ? ? ? ?return -EINVAL;
>
> [...]
>
> If the first 'if' is not true, then the second will test an
> uninitialized 'exact'.
not needed . as the cru_driver_name will always be present :).
> As far as I can tell, what we want is for 'exact' to be initialized to
> 0 (zero/false).
>
> Signed-off-by: Jesper Juhl <[email protected]>
> ---
> ?crypto/crypto_user.c | ? ?2 +-
> ?1 files changed, 1 insertions(+), 1 deletions(-)
>
> ?Compile tested only.
>
> diff --git a/crypto/crypto_user.c b/crypto/crypto_user.c
> index 16f8693..36a2af7 100644
> --- a/crypto/crypto_user.c
> +++ b/crypto/crypto_user.c
> @@ -304,7 +304,7 @@ static int crypto_del_alg(struct sk_buff *skb, struct nlmsghdr *nlh,
> ?static int crypto_add_alg(struct sk_buff *skb, struct nlmsghdr *nlh,
> ? ? ? ? ? ? ? ? ? ? ? ? ?struct nlattr **attrs)
> ?{
> - ? ? ? int exact;
> + ? ? ? int exact = 0;
> ? ? ? ?const char *name;
> ? ? ? ?struct crypto_alg *alg;
> ? ? ? ?struct crypto_user_alg *p = nlmsg_data(nlh);
> --
> 1.7.8.4
>
>
> --
> Jesper Juhl <[email protected]> ? ? ? http://www.chaosbits.net/
> Don't top-post http://www.catb.org/jargon/html/T/top-post.html
> Plain text mails only, please.
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at ?http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at ?http://www.tux.org/lkml/
On Wed, 1 Feb 2012, devendra.aaru wrote:
> On Sun, Jan 29, 2012 at 5:39 PM, Jesper Juhl <[email protected]> wrote:
> > We declare 'exact' without initializing it and then do:
> >
> > [...]
> > ? ? ? ?if (strlen(p->cru_driver_name))
> > ? ? ? ? ? ? ? ?exact = 1;
> >
> > ? ? ? ?if (priority && !exact)
> > ? ? ? ? ? ? ? ?return -EINVAL;
> >
> > [...]
> >
> > If the first 'if' is not true, then the second will test an
> > uninitialized 'exact'.
>
> not needed . as the cru_driver_name will always be present :).
If that is indeed the case, and we are guaranteed that, then it would seem
that a patch like the following would be what we want instead??
Please note that this patch is intended just for discussion, nothing else
(which is why I left out a Signed-off-by on purpose), since I've not
tested it beyond checking that it compiles, nor have I verified your claim
that cru_driver_name will always be present.
Herbert, David, any input?
Subject: [PATCH] Simplify crypto_add_alg() and crypto_alg_match()
Since cru_driver_name will always be present, the test
if (strlen(p->cru_driver_name))
exact = 1;
will always be true. So there's no need to have the test at all, we
can just unconditionally assign 1 to 'exact'.
And if 'exact' is always 1, then we'll never take the true branch of
if (priority && !exact)
return -EINVAL;
so we may as well just remove those two lines completely.
At this point we may as well just remove 'exact' entirely and just use
a hardcoded 1 in the call to crypto_alg_match().
The 'name' variable is also entirely redundant since with
cru_driver_name always being present we'll always take the first
branch in
if (strlen(p->cru_driver_name))
name = p->cru_driver_name;
else
name = p->cru_name;
and then we may as well just use 'p->cru_driver_name' itself the one
place where it is needed in the call to crypto_alg_mod_lookup().
At this point all calls to the static crypto_alg crypto_alg_match()
function pass 1 as the second argument, so there's not really any
point any more in that function having that argument, so let's remove
it and the code that tests it.
And since strlen(p->cru_driver_name) will also always be true in that
function, we can simplify the assignment to 'match'.
---
crypto/crypto_user.c | 33 ++++++++-------------------------
1 files changed, 8 insertions(+), 25 deletions(-)
diff --git a/crypto/crypto_user.c b/crypto/crypto_user.c
index 16f8693..7760c22 100644
--- a/crypto/crypto_user.c
+++ b/crypto/crypto_user.c
@@ -38,23 +38,19 @@ struct crypto_dump_info {
u16 nlmsg_flags;
};
-static struct crypto_alg *crypto_alg_match(struct crypto_user_alg *p, int exact)
+static struct crypto_alg *crypto_alg_match(struct crypto_user_alg *p)
{
struct crypto_alg *q, *alg = NULL;
down_read(&crypto_alg_sem);
list_for_each_entry(q, &crypto_alg_list, cra_list) {
- int match = 0;
+ int match;
if ((q->cra_flags ^ p->cru_type) & p->cru_mask)
continue;
- if (strlen(p->cru_driver_name))
- match = !strcmp(q->cra_driver_name,
- p->cru_driver_name);
- else if (!exact)
- match = !strcmp(q->cra_name, p->cru_name);
+ match = !strcmp(q->cra_driver_name, p->cru_driver_name);
if (match) {
alg = q;
@@ -195,7 +191,7 @@ static int crypto_report(struct sk_buff *in_skb, struct nlmsghdr *in_nlh,
if (!p->cru_driver_name)
return -EINVAL;
- alg = crypto_alg_match(p, 1);
+ alg = crypto_alg_match(p);
if (!alg)
return -ENOENT;
@@ -259,7 +255,7 @@ static int crypto_update_alg(struct sk_buff *skb, struct nlmsghdr *nlh,
if (priority && !strlen(p->cru_driver_name))
return -EINVAL;
- alg = crypto_alg_match(p, 1);
+ alg = crypto_alg_match(p);
if (!alg)
return -ENOENT;
@@ -283,7 +279,7 @@ static int crypto_del_alg(struct sk_buff *skb, struct nlmsghdr *nlh,
struct crypto_alg *alg;
struct crypto_user_alg *p = nlmsg_data(nlh);
- alg = crypto_alg_match(p, 1);
+ alg = crypto_alg_match(p);
if (!alg)
return -ENOENT;
@@ -304,28 +300,15 @@ static int crypto_del_alg(struct sk_buff *skb, struct nlmsghdr *nlh,
static int crypto_add_alg(struct sk_buff *skb, struct nlmsghdr *nlh,
struct nlattr **attrs)
{
- int exact;
- const char *name;
struct crypto_alg *alg;
struct crypto_user_alg *p = nlmsg_data(nlh);
struct nlattr *priority = attrs[CRYPTOCFGA_PRIORITY_VAL];
- if (strlen(p->cru_driver_name))
- exact = 1;
-
- if (priority && !exact)
- return -EINVAL;
-
- alg = crypto_alg_match(p, exact);
+ alg = crypto_alg_match(p);
if (alg)
return -EEXIST;
- if (strlen(p->cru_driver_name))
- name = p->cru_driver_name;
- else
- name = p->cru_name;
-
- alg = crypto_alg_mod_lookup(name, p->cru_type, p->cru_mask);
+ alg = crypto_alg_mod_lookup(p->cru_driver_name, p->cru_type, p->cru_mask);
if (IS_ERR(alg))
return PTR_ERR(alg);
--
1.7.9
--
Jesper Juhl <[email protected]> http://www.chaosbits.net/
Don't top-post http://www.catb.org/jargon/html/T/top-post.html
Plain text mails only, please.
On Wed, 1 Feb 2012, Jesper Juhl wrote:
> On Wed, 1 Feb 2012, devendra.aaru wrote:
>
> > On Sun, Jan 29, 2012 at 5:39 PM, Jesper Juhl <[email protected]> wrote:
> > > We declare 'exact' without initializing it and then do:
> > >
> > > [...]
> > > ? ? ? ?if (strlen(p->cru_driver_name))
> > > ? ? ? ? ? ? ? ?exact = 1;
> > >
> > > ? ? ? ?if (priority && !exact)
> > > ? ? ? ? ? ? ? ?return -EINVAL;
> > >
> > > [...]
> > >
> > > If the first 'if' is not true, then the second will test an
> > > uninitialized 'exact'.
> >
> > not needed . as the cru_driver_name will always be present :).
>
> If that is indeed the case, and we are guaranteed that, then it would seem
> that a patch like the following would be what we want instead??
>
> Please note that this patch is intended just for discussion, nothing else
> (which is why I left out a Signed-off-by on purpose), since I've not
> tested it beyond checking that it compiles, nor have I verified your claim
> that cru_driver_name will always be present.
>
> Herbert, David, any input?
>
>
> Subject: [PATCH] Simplify crypto_add_alg() and crypto_alg_match()
>
> Since cru_driver_name will always be present, the test
>
> if (strlen(p->cru_driver_name))
> exact = 1;
>
> will always be true. So there's no need to have the test at all, we
> can just unconditionally assign 1 to 'exact'.
>
> And if 'exact' is always 1, then we'll never take the true branch of
>
> if (priority && !exact)
> return -EINVAL;
>
> so we may as well just remove those two lines completely.
>
> At this point we may as well just remove 'exact' entirely and just use
> a hardcoded 1 in the call to crypto_alg_match().
>
> The 'name' variable is also entirely redundant since with
> cru_driver_name always being present we'll always take the first
> branch in
>
> if (strlen(p->cru_driver_name))
> name = p->cru_driver_name;
> else
> name = p->cru_name;
>
> and then we may as well just use 'p->cru_driver_name' itself the one
> place where it is needed in the call to crypto_alg_mod_lookup().
>
> At this point all calls to the static crypto_alg crypto_alg_match()
> function pass 1 as the second argument, so there's not really any
> point any more in that function having that argument, so let's remove
> it and the code that tests it.
>
> And since strlen(p->cru_driver_name) will also always be true in that
> function, we can simplify the assignment to 'match'.
>
> ---
> crypto/crypto_user.c | 33 ++++++++-------------------------
> 1 files changed, 8 insertions(+), 25 deletions(-)
>
> diff --git a/crypto/crypto_user.c b/crypto/crypto_user.c
> index 16f8693..7760c22 100644
> --- a/crypto/crypto_user.c
> +++ b/crypto/crypto_user.c
> @@ -38,23 +38,19 @@ struct crypto_dump_info {
> u16 nlmsg_flags;
> };
>
> -static struct crypto_alg *crypto_alg_match(struct crypto_user_alg *p, int exact)
> +static struct crypto_alg *crypto_alg_match(struct crypto_user_alg *p)
> {
> struct crypto_alg *q, *alg = NULL;
>
> down_read(&crypto_alg_sem);
>
> list_for_each_entry(q, &crypto_alg_list, cra_list) {
> - int match = 0;
> + int match;
>
> if ((q->cra_flags ^ p->cru_type) & p->cru_mask)
> continue;
>
> - if (strlen(p->cru_driver_name))
> - match = !strcmp(q->cra_driver_name,
> - p->cru_driver_name);
> - else if (!exact)
> - match = !strcmp(q->cra_name, p->cru_name);
> + match = !strcmp(q->cra_driver_name, p->cru_driver_name);
>
> if (match) {
Actually, we could just get rid of 'match' entirely as well and just do
if (!strcmp(q->cra_driver_name, p->cru_driver_name)) {
> alg = q;
> @@ -195,7 +191,7 @@ static int crypto_report(struct sk_buff *in_skb, struct nlmsghdr *in_nlh,
> if (!p->cru_driver_name)
> return -EINVAL;
>
> - alg = crypto_alg_match(p, 1);
> + alg = crypto_alg_match(p);
> if (!alg)
> return -ENOENT;
>
> @@ -259,7 +255,7 @@ static int crypto_update_alg(struct sk_buff *skb, struct nlmsghdr *nlh,
> if (priority && !strlen(p->cru_driver_name))
> return -EINVAL;
>
> - alg = crypto_alg_match(p, 1);
> + alg = crypto_alg_match(p);
> if (!alg)
> return -ENOENT;
>
> @@ -283,7 +279,7 @@ static int crypto_del_alg(struct sk_buff *skb, struct nlmsghdr *nlh,
> struct crypto_alg *alg;
> struct crypto_user_alg *p = nlmsg_data(nlh);
>
> - alg = crypto_alg_match(p, 1);
> + alg = crypto_alg_match(p);
> if (!alg)
> return -ENOENT;
>
> @@ -304,28 +300,15 @@ static int crypto_del_alg(struct sk_buff *skb, struct nlmsghdr *nlh,
> static int crypto_add_alg(struct sk_buff *skb, struct nlmsghdr *nlh,
> struct nlattr **attrs)
> {
> - int exact;
> - const char *name;
> struct crypto_alg *alg;
> struct crypto_user_alg *p = nlmsg_data(nlh);
> struct nlattr *priority = attrs[CRYPTOCFGA_PRIORITY_VAL];
>
> - if (strlen(p->cru_driver_name))
> - exact = 1;
> -
> - if (priority && !exact)
> - return -EINVAL;
> -
> - alg = crypto_alg_match(p, exact);
> + alg = crypto_alg_match(p);
> if (alg)
> return -EEXIST;
>
> - if (strlen(p->cru_driver_name))
> - name = p->cru_driver_name;
> - else
> - name = p->cru_name;
> -
> - alg = crypto_alg_mod_lookup(name, p->cru_type, p->cru_mask);
> + alg = crypto_alg_mod_lookup(p->cru_driver_name, p->cru_type, p->cru_mask);
> if (IS_ERR(alg))
> return PTR_ERR(alg);
>
>
--
Jesper Juhl <[email protected]> http://www.chaosbits.net/
Don't top-post http://www.catb.org/jargon/html/T/top-post.html
Plain text mails only, please.
On Wed, Feb 01, 2012 at 09:21:39PM +0100, Jesper Juhl wrote:
> On Wed, 1 Feb 2012, devendra.aaru wrote:
>
> > On Sun, Jan 29, 2012 at 5:39 PM, Jesper Juhl <[email protected]> wrote:
> > > We declare 'exact' without initializing it and then do:
> > >
> > > [...]
> > > ? ? ? ?if (strlen(p->cru_driver_name))
> > > ? ? ? ? ? ? ? ?exact = 1;
> > >
> > > ? ? ? ?if (priority && !exact)
> > > ? ? ? ? ? ? ? ?return -EINVAL;
> > >
> > > [...]
> > >
> > > If the first 'if' is not true, then the second will test an
> > > uninitialized 'exact'.
> >
> > not needed . as the cru_driver_name will always be present :).
>
> If that is indeed the case, and we are guaranteed that, then it would seem
> that a patch like the following would be what we want instead??
>
> Please note that this patch is intended just for discussion, nothing else
> (which is why I left out a Signed-off-by on purpose), since I've not
> tested it beyond checking that it compiles, nor have I verified your claim
> that cru_driver_name will always be present.
>
We get cru_driver_name from a netlink message that a user sends us.
So it depends pretty much on the user whether cru_driver_name is
set or not. Usually it is set when a user wants to instantiate
a certain algorithm driver, like "cbc(aes-asm)". If the user just
wants to instantiate the system default of an algorithm, he can
set cru_name (e.g. to "cbc(aes)") instead of cru_driver_name.
Your first patch is correct.
Thanks,
Steffen
On Thu, 2 Feb 2012, Steffen Klassert wrote:
> On Wed, Feb 01, 2012 at 09:21:39PM +0100, Jesper Juhl wrote:
> > On Wed, 1 Feb 2012, devendra.aaru wrote:
> >
> > > On Sun, Jan 29, 2012 at 5:39 PM, Jesper Juhl <[email protected]> wrote:
> > > > We declare 'exact' without initializing it and then do:
> > > >
> > > > [...]
> > > > ? ? ? ?if (strlen(p->cru_driver_name))
> > > > ? ? ? ? ? ? ? ?exact = 1;
> > > >
> > > > ? ? ? ?if (priority && !exact)
> > > > ? ? ? ? ? ? ? ?return -EINVAL;
> > > >
> > > > [...]
> > > >
> > > > If the first 'if' is not true, then the second will test an
> > > > uninitialized 'exact'.
> > >
> > > not needed . as the cru_driver_name will always be present :).
> >
> > If that is indeed the case, and we are guaranteed that, then it would seem
> > that a patch like the following would be what we want instead??
> >
> > Please note that this patch is intended just for discussion, nothing else
> > (which is why I left out a Signed-off-by on purpose), since I've not
> > tested it beyond checking that it compiles, nor have I verified your claim
> > that cru_driver_name will always be present.
> >
>
> We get cru_driver_name from a netlink message that a user sends us.
> So it depends pretty much on the user whether cru_driver_name is
> set or not. Usually it is set when a user wants to instantiate
> a certain algorithm driver, like "cbc(aes-asm)". If the user just
> wants to instantiate the system default of an algorithm, he can
> set cru_name (e.g. to "cbc(aes)") instead of cru_driver_name.
>
> Your first patch is correct.
>
Thank you for the explanation.
Can I take that to mean that I can add your Acked-by: if/when I resend the
patch?
--
Jesper Juhl <[email protected]> http://www.chaosbits.net/
Don't top-post http://www.catb.org/jargon/html/T/top-post.html
Plain text mails only, please.
On Thu, Feb 02, 2012 at 03:42:43PM +0100, Jesper Juhl wrote:
> On Thu, 2 Feb 2012, Steffen Klassert wrote:
> >
> > Your first patch is correct.
> >
> Thank you for the explanation.
>
> Can I take that to mean that I can add your Acked-by: if/when I resend the
> patch?
>
Sure,
Acked-by: Steffen Klassert <[email protected]>
On Fri, Feb 03, 2012 at 07:24:25AM +0100, Steffen Klassert wrote:
> On Thu, Feb 02, 2012 at 03:42:43PM +0100, Jesper Juhl wrote:
> > On Thu, 2 Feb 2012, Steffen Klassert wrote:
> > >
> > > Your first patch is correct.
> > >
> > Thank you for the explanation.
> >
> > Can I take that to mean that I can add your Acked-by: if/when I resend the
> > patch?
> >
>
> Sure,
>
> Acked-by: Steffen Klassert <[email protected]>
Patch applied. Thanks a lot!
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt