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

rgw: remove extra check in RGWGetObj::execute #5262

Merged
merged 1 commit into from Oct 28, 2015

Conversation

Projects
None yet
4 participants
@jmunhoz
Copy link
Contributor

jmunhoz commented Jul 16, 2015

It doesn't need to check the read_op.iterate's ret code to handle errors
in this case.

Fixes: #12352

Signed-off-by: Javier M. Mellid jmunhoz@igalia.com

@jmunhoz

This comment has been minimized.

Copy link
Contributor Author

jmunhoz commented Jul 17, 2015

@yehudasa I don't see a clear way to handle it with the current logic.

send_response_data() is handling all cases using the protected field called 'ret' internally. It looks a design decision. The checks are just used to send the response data as soon as possible in execute method (via goto jumps). The 'ret' var works as the last error seen when send_response_data() crafts the response internally.

In execute() the return code is not relevant (void method) and it doesn't check the return code coming from send_response_data() neither.

To have two different paths, I guess we would need to break and pull part of the logic from send_response_data() to execute(). It would break the current virtual approach and part of the abstraction handling responses too.

I think keeping the current approach/patch (removing the last check) could be good enough having in mind the current code. What do you think?

@yehudasa

This comment has been minimized.

Copy link
Member

yehudasa commented Jul 28, 2015

@jmunhoz I'd rather have the success path and error path separate. A more explicit approach tends to be less error prone in the long run.

@jmunhoz jmunhoz force-pushed the jmunhoz:wip-12352 branch from a7a7761 to 9bd0ae1 Jul 30, 2015

@jmunhoz

This comment has been minimized.

Copy link
Contributor Author

jmunhoz commented Jul 30, 2015

@yehudasa I pushed a new patch keeping the check, cloning the error path and just returning at the end as suggested. The success path is always a 'return 0' so I didn't add any new method to handle it.

@yehudasa

This comment has been minimized.

Copy link
Member

yehudasa commented Aug 5, 2015

@jmunhoz it's on my list, still need to review

@jmunhoz

This comment has been minimized.

Copy link
Contributor Author

jmunhoz commented Aug 7, 2015

@yehudasa don't worry! thanks for the update

rgw: add explicit success/error paths in RGWGetObj::execute()
Fixes: #12352

Signed-off-by: Javier M. Mellid <jmunhoz@igalia.com>

@jmunhoz jmunhoz force-pushed the jmunhoz:wip-12352 branch from 9bd0ae1 to dc21d8e Oct 21, 2015

@jmunhoz

This comment has been minimized.

Copy link
Contributor Author

jmunhoz commented Oct 21, 2015

@yehudasa ok, gotos in place again, added the call and return at the success path. The new commit was rebased to handle a recent conflict. Tests passing.

@yehudasa

This comment has been minimized.

Copy link
Member

yehudasa commented Oct 21, 2015

@jmunhoz yeah, that's exactly what I was aiming at

@jmunhoz

This comment has been minimized.

Copy link
Contributor Author

jmunhoz commented Oct 21, 2015

@yehudasa great!

@ghost

This comment has been minimized.

Copy link

ghost commented Oct 21, 2015

@jmunhoz please ignore the false negative from the bot ( see http://tracker.ceph.com/issues/13554 ).

@jmunhoz

This comment has been minimized.

Copy link
Contributor Author

jmunhoz commented Oct 21, 2015

@dachary ok, thanks!

yehudasa added a commit that referenced this pull request Oct 28, 2015

Merge pull request #5262 from jmunhoz/wip-12352
rgw: remove extra check in RGWGetObj::execute

Reviewed-by: Yehuda Sadeh <yehuda@redhat.com>

@yehudasa yehudasa merged commit b4cde26 into ceph:master Oct 28, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.