Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S2992897AbbEEN4P (ORCPT ); Tue, 5 May 2015 09:56:15 -0400 Received: from mx4-phx2.redhat.com ([209.132.183.25]:45767 "EHLO mx4-phx2.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S2992882AbbEEN4J (ORCPT ); Tue, 5 May 2015 09:56:09 -0400 Date: Tue, 5 May 2015 09:56:02 -0400 (EDT) From: Bob Peterson To: Fabian Frederick Cc: linux-kernel@vger.kernel.org, Alexey Dobriyan , Steven Whitehouse , cluster-devel@redhat.com Message-ID: <1787862002.12138800.1430834162686.JavaMail.zimbra@redhat.com> In-Reply-To: <1430415049-29269-1-git-send-email-fabf@skynet.be> References: <1430415049-29269-1-git-send-email-fabf@skynet.be> Subject: Re: [PATCH V2 linux-next] gfs2: convert simple_str to kstr MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-Originating-IP: [10.3.113.7] X-Mailer: Zimbra 8.0.6_GA_5922 (ZimbraWebClient - FF37 (Linux)/8.0.6_GA_5922) Thread-Topic: gfs2: convert simple_str to kstr Thread-Index: P2iT+1qEW7ZjNFg0VAKJaOWfrOVXLQ== Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4589 Lines: 171 Hi, Comments below. ----- Original Message ----- > -Remove obsolete simple_str functions. > -Return error code when kstr failed. > -This patch also calls functions corresponding to destination type. > > Thanks to Alexey Dobriyan for suggesting improvements in > block_store() and wdack_store() > > Signed-off-by: Fabian Frederick > --- > fs/gfs2/sys.c | 69 > ++++++++++++++++++++++++++++++++++++++++++----------------- > 1 file changed, 49 insertions(+), 20 deletions(-) > > diff --git a/fs/gfs2/sys.c b/fs/gfs2/sys.c > index ae8e881..3a2de91 100644 > --- a/fs/gfs2/sys.c > +++ b/fs/gfs2/sys.c > @@ -101,8 +101,11 @@ static ssize_t freeze_show(struct gfs2_sbd *sdp, char > *buf) > > static ssize_t freeze_store(struct gfs2_sbd *sdp, const char *buf, size_t > len) > { > - int error; > - int n = simple_strtol(buf, NULL, 0); > + int error, n; > + > + error = kstrtoint(buf, 0, &n); > + if (error) > + return error; > > if (!capable(CAP_SYS_ADMIN)) > return -EPERM; > @@ -134,10 +137,16 @@ static ssize_t withdraw_show(struct gfs2_sbd *sdp, char > *buf) > > static ssize_t withdraw_store(struct gfs2_sbd *sdp, const char *buf, size_t > len) > { > + int error, val; > + > if (!capable(CAP_SYS_ADMIN)) > return -EPERM; > > - if (simple_strtol(buf, NULL, 0) != 1) > + error = kstrtoint(buf, 0, &val); > + if (error) > + return error; > + > + if (val != -1) Shouldn't this be != 1, not -1 ? > return -EINVAL; > > gfs2_lm_withdraw(sdp, "withdrawing from cluster at user's request\n"); > @@ -148,10 +157,16 @@ static ssize_t withdraw_store(struct gfs2_sbd *sdp, > const char *buf, size_t len) > static ssize_t statfs_sync_store(struct gfs2_sbd *sdp, const char *buf, > size_t len) > { > + int error, val; > + > if (!capable(CAP_SYS_ADMIN)) > return -EPERM; > > - if (simple_strtol(buf, NULL, 0) != 1) > + error = kstrtoint(buf, 0, &val); > + if (error) > + return error; > + > + if (val != -1) Same here. Should that be 1, not -1? > return -EINVAL; > > gfs2_statfs_sync(sdp->sd_vfs, 0); > @@ -161,10 +176,16 @@ static ssize_t statfs_sync_store(struct gfs2_sbd *sdp, > const char *buf, > static ssize_t quota_sync_store(struct gfs2_sbd *sdp, const char *buf, > size_t len) > { > + int error, val; > + > if (!capable(CAP_SYS_ADMIN)) > return -EPERM; > > - if (simple_strtol(buf, NULL, 0) != 1) > + error = kstrtoint(buf, 0, &val); > + if (error) > + return error; > + > + if (val != -1) And again here. > return -EINVAL; > > gfs2_quota_sync(sdp->sd_vfs, 0); > @@ -181,7 +202,9 @@ static ssize_t quota_refresh_user_store(struct gfs2_sbd > *sdp, const char *buf, > if (!capable(CAP_SYS_ADMIN)) > return -EPERM; > > - id = simple_strtoul(buf, NULL, 0); > + error = kstrtou32(buf, 0, &id); > + if (error) > + return error; > > qid = make_kqid(current_user_ns(), USRQUOTA, id); > if (!qid_valid(qid)) > @@ -201,7 +224,9 @@ static ssize_t quota_refresh_group_store(struct gfs2_sbd > *sdp, const char *buf, > if (!capable(CAP_SYS_ADMIN)) > return -EPERM; > > - id = simple_strtoul(buf, NULL, 0); > + error = kstrtou32(buf, 0, &id); > + if (error) > + return error; > > qid = make_kqid(current_user_ns(), GRPQUOTA, id); > if (!qid_valid(qid)) > @@ -324,10 +349,11 @@ static ssize_t block_show(struct gfs2_sbd *sdp, char > *buf) > static ssize_t block_store(struct gfs2_sbd *sdp, const char *buf, size_t > len) > { > struct lm_lockstruct *ls = &sdp->sd_lockstruct; > - ssize_t ret = len; > - int val; > + int ret, val; > > - val = simple_strtol(buf, NULL, 0); > + ret = kstrtoint(buf, 0, &val); > + if (ret) > + return ret; > > if (val == 1) > set_bit(DFL_BLOCK_LOCKS, &ls->ls_recover_flags); > @@ -335,10 +361,9 @@ static ssize_t block_store(struct gfs2_sbd *sdp, const > char *buf, size_t len) > clear_bit(DFL_BLOCK_LOCKS, &ls->ls_recover_flags); > smp_mb__after_atomic(); > gfs2_glock_thaw(sdp); > - } else { > - ret = -EINVAL; > - } > - return ret; > + } else Just a style thing here: We never do "} else". If there's a closing bracket we always add the following open bracket ("} else {") even when there's only one line that follows. It's not incorrect, and I used to code it that way too, but I was always scolded for not having the following "{". Regards, Bob Peterson Red Hat File Systems -- 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/