Re: [PATCH RESEND 1/1] crypto API: RSA algorithm patch (kernel version 2.6.20.1)

From: Tasos Parisinos
Date: Tue Mar 20 2007 - 10:44:55 EST


Thanks for your comments

On Mon, Mar 19, 2007 at 06:22:15PM +0200, Tasos Parisinos wrote:
+static inline _i32 rsa_max(_i32 x, _i32 y)
+{
+ return (x > y)? x: y;
+}

We've got a max() already. Use tabs.


This is right, will be fixed, just hate discipline

+
+/*
+ * Module loading callback function
+ *
+ * Returns 0 on success or a negative value indicating error
+ */

This comment is not very useful.


Some of them are just bookmarks for me, i can get rid of them

+static _err __init rsa_load(void)
+{
+ _u32 i;

Can we use int and u32 instead of _err and _u32, please?

+ _err retval = RSA_NO_ERR;

And 0.

right


+ /* Pre-allocate some auxilliary mpis */
+ rsa_echo("Preallocating %lu bytes for auxilliary operands\n",
+ RSA_AUX_SIZE * RSA_AUX_COUNT * sizeof(_u32));

And printk.

i made such a printk wrapper not to mess with all the printk instances when i needed to
does this hurt, to be left as is?


+ memset(&aux, 0, sizeof(aux));
+ for(i = 0; i < RSA_AUX_COUNT; i++) {
+ retval = rsa_mpi_alloc(&aux[i], RSA_AUX_SIZE);

kmalloc, please? RSA_AUX_SIZE appears to be in bytes.


I need such a wrapper because there are other things done in rsa_mpi_alloc
than kmalloc

+ if(retval < 0)

We use "for (" and "if (" so they don't look like function calls.


right, will fix

+ goto rollback;
+ }
+ + rsa_echo("RSA cipher algorithm module initialized\n");
+ return RSA_NO_ERR;
+
+/* Free all allocated resources if any errors occur */
+rollback:
+ for(i = 0; i < RSA_AUX_COUNT; i++)
+ rsa_mpi_free(aux[i]);

kfree()


same as above, i need this wrapper


+/*
+ * Preallocate an mpi. The allocated mpi will be all-zeroed and not
+ * canonicalized.
+ *
+ * Returns 0 on success or a negative value indicating error
+ *
+ * @n: pointer pointer to the allocated mpi
+ * @limbs: number of allocated limbs (32 bit digits)
+ */
+static _err rsa_mpi_alloc(mpi ** n, _i32 limbs)

Kerneldoc style is "function_name - short description". We write
pointers as "mpi **n". These things probably all want to be named
mpi_* rather than rsa_mpi_*, as they're not specific to the RSA algorithm.

+{
+ mpi * handle;

And here.

+ rsa_debug("%s: kzalloc failed\n", __FUNCTION__);

printk.

+static _err rsa_mpi_init(mpi ** n, _u8 * str, _u32 size, _u32 xtra)

If str is an actual string, use char *str.

+ /* Allocate space for the mpi and its data */
+ s = (size / 4) + ((size % 4)? 1: 0);

Uhhh.. (size + 1) / 4?


i think (size + 3)/4


+ retval = rsa_mpi_alloc(n, s + xtra);

Is this not in bytes?

+ /* Copy the data */
+ for(i = size - 1, j = 0; i >= 0; i--, j++)
+ buf[j / 4] |= ((_u32)str[i] << ((j % 4) * 8));

Ew.

not obvious eh? ok will break it apart


+ /* Zero the xtra limbs */
+ else if(size < handle->size)
+ for(i = size; i < s; i++)
+ buf[i] = 0;

memset?

in the first case it broke my results, so i left it for later


+ return RSA_ERR_INVARG;

-EINVAL

+ buf = (*n)->data;
+ for(i = size - 1, j = 0; i >= 0; i--, j++)
+ buf[j / 4] |= ((_u32)str[i] << ((j % 4) * 8));

That mess looks familiar.


not obvious as well, will break it apart

+#define RSA_AUX_COUNT CONFIG_RSA_AUXCOUNT
+#define RSA_AUX_SIZE CONFIG_RSA_AUXSIZE

Just use the config value.

+#define RSA_MAX_U32 0xFFFFFFFF

I'm sure we've got this somewhere.


if you could tell me i will fix it

+#define RSA_NO_ERR 0
+#define RSA_ERR_INVARG -1
+#define RSA_ERR_NOMEM -2

0, -EINVAL, -ENOMEM.

+#define true 0x01
+#define false 0x00

Ew.


development leftovers, will fix


+/* Mpi utility functions */
+static _err rsa_mpi_alloc(mpi **, _i32);
+static void rsa_mpi_free(mpi *);
+static _err rsa_mpi_init(mpi **, _u8 *, _u32, _u32);
+static _err rsa_mpi_resize(mpi **, _i32, _u8);
+static _err rsa_mpi_set(mpi **, _u8 *, _u32);
+static inline _err rsa_mpi_copy(mpi **, mpi *);
+static void rsa_mpi_print(mpi *, _u8);

Why are you declaring a bunch of static functions in a header file?


i think the compiler will disagree if i dont, but i will give it a try

--
Mathematics is the supreme nostalgia of our time.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/