Re: [PATCH] cryptoloop

[Christoph Hellwig <hch@infradead.org>]

A general comment to the design of this code: imho we should rip
out the whole concept of transfer functions and go directly to
the cryptoapi algorithms - one level of unneeded abstractions
gone (the transfer funcs were never used for anything but X0R in
mainline and the interface is quite wrong as we could really
pass the page+offsets through).  If we finally decide to get rid
of this legacy junk I even volunteer to implemente a X0R cryptoapi
module :)

Some more nitpicking:

> +#include <linux/version.h>

not needed.

> +#include <linux/config.h>

dito.

> +static struct loop_func_table cryptoloop_funcs = {
> +	number:		LO_CRYPT_CRYPTOAPI,
> +	init:		cryptoloop_init,
> +	ioctl:		cryptoloop_ioctl,
> +	transfer:	cryptoloop_transfer,
> +	release:	cryptoloop_release,
> +	owner:		THIS_MODULE
> +};

please use C99-initializers.

> +	printk(KERN_INFO "cryptoloop: loaded\n");

isn't this a bit verbose?

> +	if (loop_unregister_transfer(LO_CRYPT_CRYPTOAPI))

how could this fail?

> +		printk(KERN_ERR
> +			"cryptoloop: unregistering transfer funcs failed\n");
> +
> +	printk(KERN_INFO "cryptoloop: unloaded\n");

dito.

> +	if (info->lo_encrypt_type == LO_CRYPT_CRYPTOAPI)
> +		memcpy(info64->lo_crypt_name, info->lo_name, LO_NAME_SIZE);
> +	else
> +		memcpy(info64->lo_file_name, info->lo_name, LO_NAME_SIZE);

this special-casing sounds like a bad idea.  I'd say anyone who wants
crypto support needs to use a losetup that uses the newstyle loop_info.
(Or did I get the context wrong?  diff -p helps a lot if you don't have
a recent kernel tree nearby..)