From: ebiederm@xmission.com (Eric W. Biederman)
To: Steven Rostedt <rostedt@goodmis.org>
Cc: Sasha Levin <levinsasha928@gmail.com>,
viro@zeniv.linux.org.uk, fweisbec@gmail.com, mingo@redhat.com,
a.p.zijlstra@chello.nl, paulus@samba.org,
acme@ghostprotocols.net, james.l.morris@oracle.com,
akpm@linux-foundation.org, tglx@linutronix.de,
linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-mm@kvack.org, linux-security-module@vger.kernel.org,
Eric Paris <eparis@parisplace.org>
Subject: Re: [PATCH 01/14] sysctl: provide callback for write into ctl_table entry
Date: Sun, 29 Apr 2012 19:52:37 -0700 [thread overview]
Message-ID: <m1r4v6ylqi.fsf@fess.ebiederm.org> (raw)
In-Reply-To: <1335729458.28106.247.camel@gandalf.stny.rr.com> (Steven Rostedt's message of "Sun, 29 Apr 2012 15:57:38 -0400")
Steven Rostedt <rostedt@goodmis.org> writes:
> On Sun, 2012-04-29 at 16:14 +0200, Sasha Levin wrote:
>
>> A fix for that could be having the sysctl modifying a different var,
>> and having ftrace_enabled from that under a lock, but I'm not sure if
>> it's worth the work for the cleanup.
>
> That was my original plan, but it seemed too much of a hassle than it
> was worth, as I needed to make sure the mirrored variable was in sync
> with ftrace_enabled, otherwise it could be confusing when ftrace was not
> working but sysctl showed ftrace set to 1.
I don't see the problem you are trying to solve with your patches.
What I do see is you have ignored one of the biggest problem with the
current sysctl interface in that it is not easy to plug in your own code
in the cases you need to before an update is made. (Locks permission
checks, etc).
You have also bloated struct ctl_table for no apparent reason.
This current crop of patches was just sloppy. You showed a poor
choice of function names and did not preserve necessary invariants
when changing the code. It looks like you exchanged something that
was a bit ugly for something that straight out encourages broken
behavior.
So I respectfully suggest you go back to the drawing board and figure
out a solution that makes this class of function much easier to write
in a bug free manner.
Eric
next prev parent reply other threads:[~2012-04-30 2:48 UTC|newest]
Thread overview: 23+ messages / expand[flat|nested] mbox.gz Atom feed top
2012-04-29 6:45 [PATCH 01/14] sysctl: provide callback for write into ctl_table entry Sasha Levin
2012-04-29 6:45 ` [PATCH 02/14] sched debug,sysctl: remove proc input checks out of sysctl handlers Sasha Levin
2012-04-29 6:45 ` [PATCH 03/14] sched rt,sysctl: " Sasha Levin
2012-04-30 2:32 ` Eric Paris
2012-04-29 6:45 ` [PATCH 04/14] ftrace,sysctl: " Sasha Levin
2012-04-29 6:45 ` [PATCH 05/14] sysrq,sysctl: " Sasha Levin
2012-04-29 6:45 ` [PATCH 06/14] watchdog,sysctl: remove unused external Sasha Levin
2012-04-29 6:45 ` [PATCH 07/14] watchdog,sysctl: remove proc input checks out of sysctl handlers Sasha Levin
2012-04-29 6:45 ` [PATCH 08/14] hung task,sysctl: " Sasha Levin
2012-04-29 6:45 ` [PATCH 09/14] perf,sysctl: " Sasha Levin
2012-04-29 6:45 ` [PATCH 10/14] mm,sysctl: " Sasha Levin
2012-04-29 6:45 ` [PATCH 11/14] hugetlb,sysctl: " Sasha Levin
2012-04-29 6:45 ` [PATCH 12/14] mm compaction,sysctl: " Sasha Levin
2012-04-29 6:45 ` [PATCH 13/14] security,sysctl: " Sasha Levin
2012-04-30 2:28 ` Eric Paris
2012-04-29 6:45 ` [PATCH 14/14] fs,sysctl: " Sasha Levin
2012-04-29 8:22 ` [PATCH 01/14] sysctl: provide callback for write into ctl_table entry Eric W. Biederman
2012-04-29 12:07 ` Sasha Levin
2012-04-29 14:00 ` Steven Rostedt
2012-04-29 14:14 ` Sasha Levin
2012-04-29 19:57 ` Steven Rostedt
2012-04-30 2:52 ` Eric W. Biederman [this message]
2012-04-29 12:26 ` Sasha Levin
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=m1r4v6ylqi.fsf@fess.ebiederm.org \
--to=ebiederm@xmission.com \
--cc=a.p.zijlstra@chello.nl \
--cc=acme@ghostprotocols.net \
--cc=akpm@linux-foundation.org \
--cc=eparis@parisplace.org \
--cc=fweisbec@gmail.com \
--cc=james.l.morris@oracle.com \
--cc=levinsasha928@gmail.com \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=linux-security-module@vger.kernel.org \
--cc=mingo@redhat.com \
--cc=paulus@samba.org \
--cc=rostedt@goodmis.org \
--cc=tglx@linutronix.de \
--cc=viro@zeniv.linux.org.uk \
/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).