FixForBug#2862 #4172

knrajnambiar76 · 2015-03-25T05:21:10Z

The Changes are fix to Bug # 2862

There are 3 commits

One for rbd file ie rbd.cc
Second commit groups various common files (ie strtol.cc and ceph_argparse.cc)
third commit is for test case file pool.t

Fixes: #2862 Changes related to rbd file Changes to rbd.cc ----------------- Change 1: line# 2744 to 2747 If the option is --order then do the check of its value if its less than 12 or greaterthan 25 then throw error. Correct value of --order is 12 to 25. Change 2: Removal of validation from line# 3205 to 3209 Since the check for correct value of --order is done before hence the check here is not needed. Signed-off-by: Rajesh Nambiar <rajesh.n@msystechnologies.com>

Fix#: 2862 Changes to some of the common files for command line parsing Change to ceph_argparse.cc ------------------------- Added function ceph_arg_value_type() Given an input it will determine i) If that input is an option or not ii) If input is numeric in nature or not. It will set the flag boolOption and boolNumeric appropriately. This function is called by ceph_argparse_withlonglong() and ceph_argparse_withint() to figure out if the input parameter to those functions are numeric in nature and not an option. If the input parameter to ceph_argparse_withlonglong()/ceph_argparse_withint() happens to be an option then it implies that user didn't supply value to the option. Changes to strol.cc ------------------- Changes to strict_strtoll() and strict_strtol() Both these functions reponsibility is to convert the string to long or to int. I felt it may be not be good for it to display error message within this function, rather caller of this function who has better understanding of the function's purpose can display the error message. Made change in this function to just create a generic error message,Its the caller of this function decides what to do with this message. Signed-off-by: Rajesh Nambiar <rajesh.n@msystechnologies.com>

For the local sanity test to pass this change is needed as the output message changes. Signed-off-by: Rajesh Nambiar <rajesh.n@msystechnologies.com>

loic-bot · 2015-03-25T05:40:13Z

SUCCESS: the output of run-make-check.sh on centos-7 for 0ceaea2 is http://paste2.org/1smy9n7I

Sent from GH.

Fixes: Bug # 2861 Once we have deduced image/pool/snap name from command line, validate them If invalid then bail out with appropriate error. Signed-off-by: Rajesh Nambiar <rajesh.n@msystechnologies.com>

loic-bot · 2015-03-26T08:05:10Z

SUCCESS: the output of run-make-check.sh on centos-7 for 41bdf90 is http://paste2.org/dNa5PMwt

Sent from GH.

knrajnambiar76 · 2015-03-26T08:57:49Z

I have comited fix for Bug#2861 just now
This Pull request contains fix for Bug#2862 and 2861

knrajnambiar76 · 2015-03-30T13:10:48Z

Greetings!
Any feedback regarding the changes.

dzafman · 2015-03-30T20:43:49Z

src/rbd.cc

@@ -3034,6 +3064,28 @@ if (!set_conf_param(v, p1, p2, p3)) { \
  // the relevant parts
  set_pool_image_name(imgname, (char **)&poolname,
 		      (char **)&imgname, (char **)&snapname);
+
+  if(!is_valid_name(poolname) || !is_valid_name(snapname) || !is_valid_name(imgname)) {


All these if statements should be a lot simpler. Build an error string from the names "pool" "snap" "image" and depending on how many invalid items either separate with a "," and/or "and."

Make sense.
Previous changes of mine were calling is_valid_name() for the same name
multiple times, which is not needed. Did necessary changes as suggested by
you.

On Tue, Mar 31, 2015 at 2:14 AM, David Zafman notifications@github.com
wrote:

In src/rbd.cc
#4172 (comment):

@@ -3034,6 +3064,28 @@ if (!set_conf_param(v, p1, p2, p3)) {
// the relevant parts
set_pool_image_name(imgname, (char *)&poolname,
(char *)&imgname, (char **)&snapname);
+

if(!is_valid_name(poolname) || !is_valid_name(snapname) || !is_valid_name(imgname)) {

All these if statements should be a lot simpler. Build an error string
from the names "pool" "snap" "image" and depending on how many invalid
items either separate with a "," and/or "and."

—
Reply to this email directly or view it on GitHub
https://github.com/ceph/ceph/pull/4172/files#r27429934.

Bug#: 2861 Previous changes were calling the function is_valid_name() multiple times. Made error display simpler. Signed-off-by: Rajesh Nambiar <rajesh.n@msystechnologies.com>

loic-bot · 2015-03-31T06:05:17Z

SUCCESS: the output of run-make-check.sh on centos-7 for 23d7184 is http://paste2.org/vvN3xf6E

Sent from GH.

knrajnambiar76 · 2015-03-31T12:04:50Z

Updated the code.

dzafman · 2015-03-31T17:10:11Z

src/common/strtol.cc

  errno = 0; /* To distinguish success/failure after call (see man page) */
  long long ret = strtoll(str, &endptr, base);

  if ((errno == ERANGE && (ret == LLONG_MAX || ret == LLONG_MIN))
      || (errno != 0 && ret == 0)) {
-    ostringstream oss;
-    oss << "strict_strtoll: integer underflow or overflow parsing '" << str << "'";


You should have just modified the message here and 3 other places.
Why get rid of the ostringstream that is working?

Bug # 2861 Signed off: Rajesh Nambiar <rajesh.n@msystechnologies.com>

knrajnambiar76 · 2015-04-01T07:28:05Z

Made it simpler.

loic-bot · 2015-04-01T07:59:48Z

SUCCESS: the output of run-make-check.sh on centos-7 for 29ceedf is http://paste2.org/zv9nDDtp

Sent from GH.

dzafman · 2015-04-01T17:14:09Z

Let's add test cases for the scenarios that have been improved, at least those mentioned in the 2862 tracker bug.

dzafman · 2015-04-01T18:15:39Z

Someone else made conflicting changes. You'll have to rebase and resolve the conflicts.

jdurgin · 2015-04-02T00:49:27Z

src/common/strtol.cc

-    oss << "strict_strtoll: expected integer, got: '" << str << "'";
-    *err = oss.str();
+    errStr = "The option value '";
+    errStr.append(str);


the original 'expected integer' can be pretty useful for things like pool vs poolid, which can get confused. I'd keep the specific error for this one

Ok, But I do feel from a end user's perspective these technical terms like "integer" may not make sense to him.Better may be to declare that option value is invalid.

saying it's invalid gives no information about what a valid value would be, which seems useful to me for this case. The end user for the rbd cli tool is likely familiar with the word integer, since they're probably a storage admin/ops/dev rather than an end user of whatever service they're deploying ceph underneath.

jdurgin · 2015-04-02T01:26:00Z

I like the detailed and well-isolated commits. It'd be good to match the existing coding style - a few differences I notice:

use underscores for var_names instead of camelCase
put a space between if and (
put spaces after commas, and around operators like <<
any if with an else clause where either part is multiple lines should have {} for both clauses
The coding style guide is here: https://github.com/ceph/ceph/blob/master/CodingStyle

Conflicts: src/common/ceph_argparse.cc src/rbd.cc

loic-bot · 2015-04-02T08:24:54Z

SUCCESS: the output of run-make-check.sh on centos-7 for bb449dd is http://paste2.org/I502c0vM

Sent from GH.

knrajnambiar76 · 2015-04-02T12:05:37Z

Greetings.
I have Merged the changes and also updated the code as per Ceph Coding style. I have put my thoughts to the valuable comments which you have written. If some further changes are needed please let me know.
Regarding the addition of test case, within ceph/src/test/cli/rbd directory Iam seeing files "not-enough-args.t", "invalid-snap-usage.t" and "help.t" none of these are suppose to cover invalid argument cases. Which means there is need to add new file for checking invalid cases, However I am not able to figure out, to which file I need to make an entry so that particular new file gets executed along with other *.t files when build happens.

1) Reverted back changes related to name validation for Bug # 2861 due to utf 8 and compatibility issues. 2) Change in message in strtol.cc Signed Off: Rajesh Nambiar <rajesh.n@msystechnologies.com>

knrajnambiar76 · 2015-04-03T08:55:35Z

Josh, I have done the necessary changes please have a look.

loic-bot · 2015-04-03T09:10:03Z

SUCCESS: the output of run-make-check.sh on centos-7 for e64808a is http://paste2.org/93W0h7v4

Sent from GH.

jdurgin · 2015-04-06T22:46:20Z

Thanks Rajesh, looks good! Could you squash the extra commits from review into the original 3?

loic-bot · 2015-04-07T06:50:12Z

SUCCESS: the output of run-make-check.sh on centos-7 for d206384 is http://paste2.org/vbjXLp44

Sent from GH.

knrajnambiar76 · 2015-04-07T10:19:43Z

The rebase seems to be failing

ubuntu@Host-CephAdmin:~~/Final/ceph$ git rebase -i HEAD~~15

I did pick of the original 3 commits and squash of commits 0959204
0959204
and 7b5e1ab
7b5e1ab
also removed commits 267bdc4
267bdc4,
96ed40d
96ed40d
and 9644601
9644601

however rebase fails with below error, the Checkin 6076233 is not
associated with me.

[detached HEAD 4318acb] Fix to some of the command line parsing (including
rbd)
2 files changed, 91 insertions(+), 19 deletions(-)
error: could not apply 6076233... crc32c: add aarch64 optimized crc32c
implementation

When you have resolved this problem run "git rebase --continue".
If you would prefer to skip this patch, instead run "git rebase --skip".
To check out the original branch and stop rebasing run "git rebase --abort".
Could not apply 6076233... crc32c: add aarch64 optimized crc32c
implementation

ubuntu@Host-CephAdmin:~/Final/ceph$ git status

Not currently on any branch.

Changes to be committed:

(use "git reset HEAD ..." to unstage)

modified: configure.ac

modified: m4/ax_arm.m4

modified: src/arch/arm.c

modified: src/arch/arm.h

modified: src/common/Makefile.am

modified: src/common/crc32c.cc

new file: src/common/crc32c_aarch64.c

new file: src/common/crc32c_aarch64.h

modified: src/test/common/test_crc32c.cc

modified: src/test/test_arch.cc

Unmerged paths:

(use "git reset HEAD ..." to unstage)

(use "git add/rm ..." as appropriate to mark resolution)

added by us: src/gmock

On Tue, Apr 7, 2015 at 4:16 AM, Josh Durgin notifications@github.com
wrote:

Thanks Rajesh, looks good! Could you squash the extra commits from review
into the original 3?

—
Reply to this email directly or view it on GitHub
#4172 (comment).

jdurgin · 2015-04-08T07:04:50Z

Sometimes it's easiest to reset --mixed and recreate the commits. I did that and pushed the result to wip-2862 in the ceph repo. I'll merge once it passes through testing.

knrajnambiar76 · 2015-04-08T07:12:43Z

Looks like I messed the repository. I deleted that old one and created new
one ( pull requst # 4299), it contains all the necessary changes.

On Wed, Apr 8, 2015 at 12:35 PM, Josh Durgin notifications@github.com
wrote:

Sometimes it's easiest to reset --mixed and recreate the commits. I did
that and pushed the result to wip-2862 in the ceph repo. I'll merge once it
passes through testing.

—
Reply to this email directly or view it on GitHub
#4172 (comment).

knrajnambiar76 · 2015-04-08T07:17:18Z

In case if you have already taken the changes then its great, else to avoid
confusion I created a new request
#4299

which contains all the needed changes.

Rgds
Rajesh

On Wed, Apr 8, 2015 at 12:35 PM, Josh Durgin notifications@github.com
wrote:

Sometimes it's easiest to reset --mixed and recreate the commits. I did
that and pushed the result to wip-2862 in the ceph repo. I'll merge once it
passes through testing.

—
Reply to this email directly or view it on GitHub
#4172 (comment).

rajesh added 3 commits March 25, 2015 10:13

TestCase: Change in testcase output

a3530eb

For the local sanity test to pass this change is needed as the output message changes. Signed-off-by: Rajesh Nambiar <rajesh.n@msystechnologies.com>

liewegas added bug-fix rbd labels Mar 26, 2015

liewegas assigned jdurgin Mar 26, 2015

rbd: Validation of image/pool/snap name for create action

9644601

Fixes: Bug # 2861 Once we have deduced image/pool/snap name from command line, validate them If invalid then bail out with appropriate error. Signed-off-by: Rajesh Nambiar <rajesh.n@msystechnologies.com>

dzafman reviewed Mar 30, 2015
View reviewed changes

rbd: Optimized error message display as suggested by David Zafman

96ed40d

Bug#: 2861 Previous changes were calling the function is_valid_name() multiple times. Made error display simpler. Signed-off-by: Rajesh Nambiar <rajesh.n@msystechnologies.com>

dzafman reviewed Mar 31, 2015
View reviewed changes

Made error code display simpler

267bdc4

Bug # 2861 Signed off: Rajesh Nambiar <rajesh.n@msystechnologies.com>

jdurgin reviewed Apr 2, 2015
View reviewed changes

Merge remote-tracking branch 'upstream/master'

8e43bf5

Conflicts: src/common/ceph_argparse.cc src/rbd.cc

Changes as suggested by Josh

0959204

1) Reverted back changes related to name validation for Bug # 2861 due to utf 8 and compatibility issues. 2) Change in message in strtol.cc Signed Off: Rajesh Nambiar <rajesh.n@msystechnologies.com>

Aligned to Ceph Coding Style

7b5e1ab

jdurgin closed this Apr 8, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FixForBug#2862 #4172

FixForBug#2862 #4172

knrajnambiar76 commented Mar 25, 2015

loic-bot commented Mar 25, 2015

loic-bot commented Mar 26, 2015

knrajnambiar76 commented Mar 26, 2015

knrajnambiar76 commented Mar 30, 2015

dzafman Mar 30, 2015

knrajnambiar76 Mar 31, 2015

loic-bot commented Mar 31, 2015

knrajnambiar76 commented Mar 31, 2015

dzafman Mar 31, 2015

knrajnambiar76 commented Apr 1, 2015

loic-bot commented Apr 1, 2015

dzafman commented Apr 1, 2015

dzafman commented Apr 1, 2015

jdurgin Apr 2, 2015

knrajnambiar76 Apr 2, 2015

jdurgin Apr 2, 2015

jdurgin commented Apr 2, 2015

loic-bot commented Apr 2, 2015

knrajnambiar76 commented Apr 2, 2015

knrajnambiar76 commented Apr 3, 2015

loic-bot commented Apr 3, 2015

jdurgin commented Apr 6, 2015

loic-bot commented Apr 7, 2015

knrajnambiar76 commented Apr 7, 2015

jdurgin commented Apr 8, 2015

knrajnambiar76 commented Apr 8, 2015

knrajnambiar76 commented Apr 8, 2015

FixForBug#2862 #4172

FixForBug#2862 #4172

Conversation

knrajnambiar76 commented Mar 25, 2015

loic-bot commented Mar 25, 2015

loic-bot commented Mar 26, 2015

knrajnambiar76 commented Mar 26, 2015

knrajnambiar76 commented Mar 30, 2015

dzafman Mar 30, 2015

Choose a reason for hiding this comment

knrajnambiar76 Mar 31, 2015

Choose a reason for hiding this comment

loic-bot commented Mar 31, 2015

knrajnambiar76 commented Mar 31, 2015

dzafman Mar 31, 2015

Choose a reason for hiding this comment

knrajnambiar76 commented Apr 1, 2015

loic-bot commented Apr 1, 2015

dzafman commented Apr 1, 2015

dzafman commented Apr 1, 2015

jdurgin Apr 2, 2015

Choose a reason for hiding this comment

knrajnambiar76 Apr 2, 2015

Choose a reason for hiding this comment

jdurgin Apr 2, 2015

Choose a reason for hiding this comment

jdurgin commented Apr 2, 2015

loic-bot commented Apr 2, 2015

knrajnambiar76 commented Apr 2, 2015

knrajnambiar76 commented Apr 3, 2015

loic-bot commented Apr 3, 2015

jdurgin commented Apr 6, 2015

loic-bot commented Apr 7, 2015

knrajnambiar76 commented Apr 7, 2015

Not currently on any branch.

Changes to be committed:

(use "git reset HEAD ..." to unstage)

modified: configure.ac

modified: m4/ax_arm.m4

modified: src/arch/arm.c

modified: src/arch/arm.h

modified: src/common/Makefile.am

modified: src/common/crc32c.cc

new file: src/common/crc32c_aarch64.c

new file: src/common/crc32c_aarch64.h

modified: src/test/common/test_crc32c.cc

modified: src/test/test_arch.cc

Unmerged paths:

(use "git reset HEAD ..." to unstage)

(use "git add/rm ..." as appropriate to mark resolution)

added by us: src/gmock

jdurgin commented Apr 8, 2015

knrajnambiar76 commented Apr 8, 2015

knrajnambiar76 commented Apr 8, 2015