search

OpenAoE / aoe

aoe driver for Linux with backward compatibility build system
Other
26 stars 8 forks source link

driver chokes if bufcnt < 2 #7

Open mischief opened 8 years ago

mischief commented 8 years ago

if a aoe target sends a bufcnt of 0 or 1, the driver will choke with no indication of the problem.

the issue shows up during handling the config query response. if bufcnt is 1, t->maxout is calculated as 0 at aoecmd.c#L1553. later, newframe is called to send the ata identify command, but the frame is NULL at aoecmd.c#L284 because of the t->nout < t->maxout check.

maybe there should be a warning print in the driver if this occurs. it took me a long time to debug my target implementation because of this small problem :)

ecashin commented 8 years ago

Thanks for the suggestion. I am sick now but hope to look into that soon.

mischief commented 8 years ago

sorry to hear it, take care :-)

ecashin commented 8 years ago

On closer examination, it looks like the driver doesn't become unusable, and the kernel doesn't crash, but the new device is unusable. If undefined behavior occurs when the AoE target is behaving outside the spec, that kind of makes sense, as long as it only affects the misbehaving target.

However, as you discovered, the aoe driver could help make it easier for people to create or modify new targets if it logged a warning.

In the past, the Linux Kernel Mailing List (LKML) developers have been wary of doing that without rate limiting. You can see other parts in the aoe driver where we use net_ratelimit() to avoid saturating the logging system with warnings.

If all that sounds reasonable to you, I'll add such a rate-limited warning. And please let me know if there are other places like this where we could encourage AoE target developers without complicating the code.

mischief commented 8 years ago

I should have been more specific - it only chokes up on the misbehaving target.

A rate limited message sounds fine. On Mar 16, 2016 5:35 PM, "Ed Cashin" notifications@github.com wrote:

On closer examination, it looks like the driver doesn't become unusable, and the kernel doesn't crash, but the new device is unusable. If undefined behavior occurs when the AoE target is behaving outside the spec, that kind of makes sense, as long as it only affects the misbehaving target.

However, as you discovered, the aoe driver could help make it easier for people to create or modify new targets if it logged a warning.

In the past, the Linux Kernel Mailing List (LKML) developers have been wary of doing that without rate limiting. You can see other parts in the aoe driver where we use net_ratelimit() to avoid saturating the logging system with warnings.

If all that sounds reasonable to you, I'll add such a rate-limited warning. And please let me know if there are other places like this where we could encourage AoE target developers without complicating the code.

— You are receiving this because you authored the thread. Reply to this email directly or view it on GitHub https://github.com/OpenAoE/aoe/issues/7#issuecomment-197622625

ecashin commented 8 years ago

Hi, @mischief. Can you please check that the changes here:

https://github.com/OpenAoE/aoe/pull/9

... lead to the desired behavior? Hopefully you can comment on the pull request, but I've only used Stash/Bitbucket web-based pull request discussions before, so I'm not sure.