From: Jes Sorensen <Jes.Sorensen@redhat.com>
To: NeilBrown <neilb@suse.de>
Cc: linux-raid@vger.kernel.org
Subject: Re: [PATCH 00/13] Add missing handling of malloc() failure
Date: Thu, 27 Oct 2011 09:39:41 +0200 [thread overview]
Message-ID: <4EA90ABD.40003@redhat.com> (raw)
In-Reply-To: <20111027154926.4d5e8fef@notabene.brown>
On 10/27/11 06:49, NeilBrown wrote:
>> Hi,
>> >
>> > I cam across a couple of these last week while I was chasing the
>> > assemble problem of IMSM raids, so I started going through the code to
>> > fix up some more of these.
>> >
>> > This is just low hanging fruit, and there is more to be done, but at
>> > least it's a few fixes.
> Hi,
>
> I'm not at all sure this sort of thing is worth it.
> There is no guarantee that the memory is actually there when malloc returns
> non-NULL. You might only get a page mapped when you access the memory, and
> equally the failure could happen at that time.
>
> If there is something genuinely useful that can be done when malloc fails,
> the it might make sense, but just printing a message an exiting seems
> pointless.
>
> If we were to go that route, I would probably want to use a #define to
> replace everything with a wrapper (xmalloc??) that printed a message and
> failed.
>
> Do you have a strong reason to add these checks?
Hi Neil,
There are a couple of reasons for this. You are right that things could
happen during touching the page at first, but in general malloc() is
supposed to fail if the system is unable to provide the pages.
The main reason why I think this is worth doing is for support reasons.
While in most cases these cases will never fail, I much prefer a user to
find out of memory messages in their log files or on the terminal, than
having them report a segfault which we need to investigate the reason
of. Having better error messages in place should reduce the cost of this.
Second, there are cases where malloc() fails where I think we really
need to try and unwind, rather than possibly leaving raid or metadata in
an inconsistent state. Passing errors back should help with this.
Third I think it is good practice not to leave error codes unchecked. In
the cases where I let the code exit() on error, I was pretty much
following the precedence in the same function I added the check to. I
don't think it is ideal, but I do think it is a starting point.
Some of the bits in my patchset could definitely handle the errors
better, but I still don't know all the code that well, so I think it is
a start. Getting error handling consistent across the board is a big
project and I don't think it is realistic to do it all in one go.
Cheers,
Jes
prev parent reply other threads:[~2011-10-27 7:39 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2011-10-26 15:30 [PATCH 00/13] Add missing handling of malloc() failure Jes.Sorensen
2011-10-26 15:30 ` [PATCH 01/13] count_active() catch " Jes.Sorensen
2011-10-26 15:30 ` [PATCH 02/13] Catch malloc() failures Jes.Sorensen
2011-10-26 15:30 ` [PATCH 03/13] Try to catch malloc() failures in Assemble.c Jes.Sorensen
2011-10-26 15:30 ` [PATCH 04/13] Attempt to catch malloc() failure in Detail.c Jes.Sorensen
2011-10-26 15:30 ` [PATCH 05/13] Handle malloc() failure in Examine.c Jes.Sorensen
2011-10-26 15:30 ` [PATCH 06/13] Handle malloc() errors in Manage.c Jes.Sorensen
2011-10-26 15:30 ` [PATCH 07/13] Fix malloc() failure handling in Monitor.c Jes.Sorensen
2011-10-26 15:30 ` [PATCH 08/13] Handle malloc() failures in devline() Jes.Sorensen
2011-10-26 15:30 ` [PATCH 09/13] Fix malloc handling in dlink.c Jes.Sorensen
2011-10-26 15:30 ` [PATCH 10/13] Handle malloc() failure in Grow.c Jes.Sorensen
2011-10-26 15:30 ` [PATCH 11/13] Handle malloc() failure in mdopen.c Jes.Sorensen
2011-10-26 15:30 ` [PATCH 12/13] Handle malloc() failure in mdstat.c Jes.Sorensen
2011-10-26 15:30 ` [PATCH 13/13] Catch malloc() failure Jes.Sorensen
2011-10-27 4:49 ` [PATCH 00/13] Add missing handling of " NeilBrown
2011-10-27 7:39 ` Jes Sorensen [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=4EA90ABD.40003@redhat.com \
--to=jes.sorensen@redhat.com \
--cc=linux-raid@vger.kernel.org \
--cc=neilb@suse.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).