Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

mount: do not print "unknown" option to kclient #12465

Merged
merged 1 commit into from Jan 12, 2017
Merged

Conversation

jcsp
Copy link
Contributor

@jcsp jcsp commented Dec 13, 2016

If these options were really unknown to the kernel client,
we would quickly find out when it returned an error. No need
to mention them in the output of the userspace tool.

Fixes: http://tracker.ceph.com/issues/18159
Signed-off-by: John Spray john.spray@redhat.com

@jcsp jcsp added bug-fix cephfs Ceph File System labels Dec 13, 2016
Copy link
Member

@gregsfortytwo gregsfortytwo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed-by: Greg Farnum gfarnum@redhat.com

Let's run it through the testing suite to make sure we don't have any text-based tests which fail before merging, though.

@@ -201,8 +201,6 @@ static char *parse_options(const char *data, int *filesys_flags)
skip = 0;
} else {
skip = 0;
if (verboseflag)
printf("ceph: Unknown mount option %s\n",data);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that I look, I don't know... the verboseflag was set, so I think it's entirely reasonable to inform that the mount helper did not recognize the option. If we're worried about those sorts of messages then we shouldn't specify -v when mounting.

The message could be more clear though. For one thing, we should probably make it clear that it comes from "mount.ceph" and not "ceph". It should also go to stderr, IMO. Maybe:

fprintf(stderr, "mount.ceph: unrecognized mount option \"%s\", passing to kernel.\n", data);

...just to make it clear that it's not necessarily a problem.

The old message sounded worse than it really was,
and was going to stdout instead of stderr.

Fixes: http://tracker.ceph.com/issues/18159
Signed-off-by: John Spray <john.spray@redhat.com>
@jcsp
Copy link
Contributor Author

jcsp commented Dec 25, 2016

Updated

@jcsp
Copy link
Contributor Author

jcsp commented Jan 6, 2017

@jtlayton look good?

Copy link
Contributor

@jtlayton jtlayton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

@jcsp jcsp merged commit 6048704 into ceph:master Jan 12, 2017
@jcsp jcsp deleted the wip-18159 branch January 12, 2017 00:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug-fix cephfs Ceph File System
Projects
None yet
3 participants