| Message ID | 1365141303-10571-3-git-send-email-anand.jain@oracle.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
On Fri, Apr 05, 2013 at 01:54:56PM +0800, Anand Jain wrote: > --- a/cmds-balance.c > +++ b/cmds-balance.c > @@ -662,8 +662,12 @@ static int cmd_balance_status(int argc, char **argv) > close(fd); > > if (ret < 0) { > + if (e == ENOTCONN) { > + printf("No balance found on '%s'\n", path); > + return 0; > + } > fprintf(stderr, "ERROR: balance status on '%s' failed - %s\n", > - path, (e == ENOTCONN) ? "Not in progress" : strerror(e)); I'm not sure if we want to change the error code if balance is not in progress. That's the only way to find out if it is so. Let's say that I have this shell code as a balance monitor and expect it to finish when the balance finishes: while btrfs fi balance status; do sleep 5; done not possible after your change. What we can do is to differentiate the status by a different error code number, let's say 1 for an error and 2 for 'not in progress'. > + path, strerror(e)); > return 19; > } -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 04/12/2013 11:57 PM, David Sterba wrote: > On Fri, Apr 05, 2013 at 01:54:56PM +0800, Anand Jain wrote: >> --- a/cmds-balance.c >> +++ b/cmds-balance.c >> @@ -662,8 +662,12 @@ static int cmd_balance_status(int argc, char **argv) >> close(fd); >> >> if (ret < 0) { >> + if (e == ENOTCONN) { >> + printf("No balance found on '%s'\n", path); >> + return 0; >> + } >> fprintf(stderr, "ERROR: balance status on '%s' failed - %s\n", >> - path, (e == ENOTCONN) ? "Not in progress" : strerror(e)); > > I'm not sure if we want to change the error code if balance is not in > progress. That's the only way to find out if it is so. > > Let's say that I have this shell code as a balance monitor and expect it > to finish when the balance finishes: > > while btrfs fi balance status; do > sleep 5; > done > > not possible after your change. What we can do is to differentiate the > status by a different error code number, let's say 1 for an error and 2 > for 'not in progress'. > >> + path, strerror(e)); >> return 19; >> } Thanks for the review. good point. Hope the below return code will suffice /* * return codes: * -1 : Error, failed to know if there is any pending balance * 1 : Successful to know status of a pending balance * 0 : when there is no pending balance or completed */ OR do we prefer /* * return codes: * -1 : Error, failed to know if there is any pending balance * 0 : Successful to know status of a pending balance * 1 : when there is no pending balance or completed */ I am fine with either. Thanks, Anand -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/cmds-balance.c b/cmds-balance.c index f5dc317..a690301 100644 --- a/cmds-balance.c +++ b/cmds-balance.c @@ -662,8 +662,12 @@ static int cmd_balance_status(int argc, char **argv) close(fd); if (ret < 0) { + if (e == ENOTCONN) { + printf("No balance found on '%s'\n", path); + return 0; + } fprintf(stderr, "ERROR: balance status on '%s' failed - %s\n", - path, (e == ENOTCONN) ? "Not in progress" : strerror(e)); + path, strerror(e)); return 19; }
Having no balance running/ paused/completed is a normal situation, so the current output message should be positive with return val zero. As of now: Signed-off-by: Anand Jain <anand.jain@oracle.com> --- cmds-balance.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)