XrdHttp: The deletion of a non-empty directory returns a 405 error code instead of 500 #2016
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
abh3
requested changes
May 26, 2023
src/XProtocol/XProtocol.hh
Outdated
| // In the case one tries to delete a non-empty directory | ||
| // we have decided that until the next major release | ||
| // the kXR_ItExists flag will be returned | ||
| case ENOTEMPTY: return kXR_ItExists; |
There was a problem hiding this comment.
What should happen here is that this case needs to be put ahead if the kXR_ItExists case as a falthrough. That is
using [[fallthrough]]; to indicate that the fallthrough is correct.
There was a problem hiding this comment.
I do not understand 🤔 Is it a fallthrough case? I just return kXR_ItExists when the errno code is ENOTEMPTY. Each switch case has a return value, so there's no need for a [[fallthrough]]; attribute, isn't it?
|
Yes, it's a fallthrough case which makes it much more obvious what is
happening. Don't forget to specify [[falthrough]]
…On Fri, 26 May 2023, ccaffy wrote:
@ccaffy commented on this pull request.
> @@ -1391,6 +1391,10 @@ static int mapError(int rc)
case ETIMEDOUT: return kXR_ReqTimedOut;
case EBADF: return kXR_FileNotOpen;
case ECANCELED: return kXR_Cancelled;
+ // In the case one tries to delete a non-empty directory
+ // we have decided that until the next major release
+ // the kXR_ItExists flag will be returned
+ case ENOTEMPTY: return kXR_ItExists;
I do not understand 🤔 Is it a fallthrough case? I just return kXR_ItExists when the errno code is ENOTEMPTY. Each switch case has a return value, so there's no need for a `[[fallthrough]];` attribute, isn't it?
--
Reply to this email directly or view it on GitHub:
#2016 (comment)
You are receiving this because your review was requested.
Message ID: ***@***.***>
|
ccaffy
force-pushed
the
1896-webdav-method-delete-should-return-405
branch
2 times, most recently
from
e916f41
to
3b87b77
Compare
abh3
previously requested changes
May 30, 2023
ccaffy
force-pushed
the
1896-webdav-method-delete-should-return-405
branch
from
3b87b77
to
a979cae
Compare
amadio
reviewed
May 30, 2023
amadio
reviewed
May 30, 2023
…de instead of 500 The mapping between errno error codes and XRoot protocol error code has also been changed. ENOTEMPTY is mapped to kXR_ItExists
ccaffy
force-pushed
the
1896-webdav-method-delete-should-return-405
branch
from
a979cae
to
ad9d26b
Compare
|
I'll merge this in 12 hours or so once the CI completes. Thank you Cedric! |
|
My pleasure :) |
amadio
dismissed
abh3’s stale review
All change requests have been addressed, merging.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The mapping between errno error codes and XRoot protocol error code has also been changed. ENOTEMPTY is mapped to kXR_ItExists