ATA over ethernet swapping and obfuscated code

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.DeleteThis@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);


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);
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;
}

Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo.DeleteThis@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Hi Pavel,

On Tue, 31 Jul 2007 15:58:31 +0200 Pavel Machek <pavel DeleteThis @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 DeleteThis @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 DeleteThis @vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/