-
Notifications
You must be signed in to change notification settings - Fork 6k
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
osdc: clean up osd_command/start_mon_command interfaces #13727
Conversation
/home/jenkins-build/build/workspace/ceph-pull-requests/src/librados/RadosClient.cc: In member function ‘int librados::RadosClient::osd_command(int, std::vector<std::basic_string >&, const bufferlist&, ceph::bufferlist*, std::string*)’: |
Oops, some of this got lost in another commit on #13725, fixing... |
src/librados/librados.cc
Outdated
return client->osd_command(osdid, cmdvec, inbl, outbl, outs); | ||
client->osd_command(osdid, cmdvec, inbl, outbl, outs); | ||
|
||
return 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is eating the return value
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i concur with @liewegas
@jcsp this breaks this test:
2017-03-08T15:57:53.322 INFO:tasks.workunit.client.0.smithi156.stdout: api_cmd: [ RUN ] LibRadosCmd.OSDCmdPP
2017-03-08T15:57:53.323 INFO:tasks.workunit.client.0.smithi156.stdout: api_cmd: /build/ceph-12.0.0-1141-gc7d986e/src/test/librados/cmd.cc:146: Failure
2017-03-08T15:57:53.323 INFO:tasks.workunit.client.0.smithi156.stdout: api_cmd: Value of: r == -22 || r == -ENXIO
2017-03-08T15:57:53.323 INFO:tasks.workunit.client.0.smithi156.stdout: api_cmd: Actual: false
2017-03-08T15:57:53.323 INFO:tasks.workunit.client.0.smithi156.stdout: api_cmd: Expected: true
2017-03-08T15:57:53.323 INFO:tasks.workunit.client.0.smithi156.stdout: api_cmd: [ FAILED ] LibRadosCmd.OSDCmdPP (15 ms)
src/librados/librados.cc
Outdated
if (ret) { | ||
return ret; | ||
} | ||
client.mon_command(cmd, inbl, &outbl, &outstring); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this one too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Objecter calls changed; the RadosClient ones didn't
src/librados/librados.cc
Outdated
return client->osd_command(osdid, cmdvec, inbl, outbl, outs); | ||
client->osd_command(osdid, cmdvec, inbl, outbl, outs); | ||
|
||
return 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i concur with @liewegas
@jcsp this breaks this test:
2017-03-08T15:57:53.322 INFO:tasks.workunit.client.0.smithi156.stdout: api_cmd: [ RUN ] LibRadosCmd.OSDCmdPP
2017-03-08T15:57:53.323 INFO:tasks.workunit.client.0.smithi156.stdout: api_cmd: /build/ceph-12.0.0-1141-gc7d986e/src/test/librados/cmd.cc:146: Failure
2017-03-08T15:57:53.323 INFO:tasks.workunit.client.0.smithi156.stdout: api_cmd: Value of: r == -22 || r == -ENXIO
2017-03-08T15:57:53.323 INFO:tasks.workunit.client.0.smithi156.stdout: api_cmd: Actual: false
2017-03-08T15:57:53.323 INFO:tasks.workunit.client.0.smithi156.stdout: api_cmd: Expected: true
2017-03-08T15:57:53.323 INFO:tasks.workunit.client.0.smithi156.stdout: api_cmd: [ FAILED ] LibRadosCmd.OSDCmdPP (15 ms)
The int return was misleading because it didn't really have any error cases. We sometimes use ints in APIs in case we want to return an error in future, but Objecter is an internal interface. Signed-off-by: John Spray <john.spray@redhat.com>
The int return was misleading because it didn't really have any error cases. We sometimes use ints in APIs in case we want to return an error in future, but MonClient is an internal interface. Signed-off-by: John Spray <john.spray@redhat.com>
Signed-off-by: John Spray <john.spray@redhat.com>
Signed-off-by: John Spray <john.spray@redhat.com>
Signed-off-by: John Spray <john.spray@redhat.com>
Signed-off-by: John Spray <john.spray@redhat.com>
This is hopefully good now |
Functions that can't return errors should return void. I was using these interfaces from #13725 and didn't like writing in even more places where we would either assert(r==0) or do an error handling branch that would never get taken.