Re: ATA over ethernet swapping and obfuscated code

[Sébastien Dugué <sebastien.dugue@bull.net>]

  Hi Pavel,

On Tue, 31 Jul 2007 15:58:31 +0200 Pavel Machek <pavel@ucw.cz> wrote:

> Hi!
> 
> I wanted to know if it is possible/okay to swap over AOE... 
> 
> According to
> http://www.coraid.com/support/linux/EtherDrive-2.6-HOWTO-5.html#ss5.20
> .. it runs OOM even during normal use, so I guess swapping over it is
> no-no?
> 
> Can I build both client and server for these using free software?
> 
> In the process, I looked at the aoe code, and parts of it look like
> obfuscated C contest. The use of switch() as an if was particulary
> creative; I'm not even sure if I translated it properly... can you
> take a look?
> 
> (Patch is 
> 
> Signed-off-by: Pavel Machek <pavel@suse.cz>
> 
> but I did not even compile test it)
> 
> diff --git a/drivers/block/aoe/aoedev.c b/drivers/block/aoe/aoedev.c
> index 05a9719..38ba35d 100644
> --- a/drivers/block/aoe/aoedev.c
> +++ b/drivers/block/aoe/aoedev.c
> @@ -64,29 +64,26 @@ aoedev_newdev(ulong nframes)
>  
>  	d = kzalloc(sizeof *d, GFP_ATOMIC);
>  	f = kcalloc(nframes, sizeof *f, GFP_ATOMIC);
> - 	switch (!d || !f) {
> - 	case 0:
> - 		d->nframes = nframes;
> - 		d->frames = f;
> - 		e = f + nframes;
> - 		for (; f<e; f++) {
> - 			f->tag = FREETAG;
> - 			f->skb = new_skb(ETH_ZLEN);
> - 			if (!f->skb)
> - 				break;
> - 		}
> - 		if (f == e)
> - 			break;
> + 	if (!d || !f) {
> +		kfree(f);
> +		kfree(d);
> +		return NULL;
> +	}
> +
> +	d->nframes = nframes;
> +	d->frames = f;
> +	e = f + nframes;
> +	for (; f<e; f++) {
> +		f->tag = FREETAG;
> +		f->skb = new_skb(ETH_ZLEN);
> +		if (!f->skb)
> +			break;
> +	}
> +	if (f != e) {
>   		while (f > d->frames) {
>   			f--;
>   			dev_kfree_skb(f->skb);
>   		}
> - 	default:
> - 		if (f)
> - 			kfree(f);
> - 		if (d)
> - 			kfree(d);
> -		return NULL;
>  	}
>  	INIT_WORK(&d->work, aoecmd_sleepwork);
>  	spin_lock_init(&d->lock);

  Creative it is.

> 
> 
> aoedev_by_sysminor_m() returns with spinlock held in error case; I
> guess that's bad.
> 
> struct aoedev *
> aoedev_by_sysminor_m(ulong sysminor, ulong bufcnt)
> {
> 	struct aoedev *d;
> 	ulong flags;
> 
> 	spin_lock_irqsave(&devlist_lock, flags);
> 
> 	for (d=devlist; d; d=d->next)
> 		if (d->sysminor == sysminor)
> 			break;
> 
> 	if (d == NULL) {
> 		d = aoedev_newdev(bufcnt);
> 	 	if (d == NULL) {
> 			spin_unlock_irqrestore(&devlist_lock, flags);

~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ what about here

> 			printk(KERN_INFO "aoe: aoedev_newdev
> failure.\n");
> 			return NULL;
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ here
> 		}
> 		d->sysminor = sysminor;
> 		d->aoemajor = AOEMAJOR(sysminor);
> 		d->aoeminor = AOEMINOR(sysminor);
> 	}
> 
> 	spin_unlock_irqrestore(&devlist_lock, flags);
> 	return d;
> }
> 

  Sébastien.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/