 |
 |
@abh3 requested changes on this pull request.
I think we still need some changes. Nothing drastic.
In src/XrdHttp/README-CKSUM.md:
> @@ -0,0 +1,15 @@ +In the case XrdHttp has been configured, it is important that the configuration contains at least one digest algorithm registered in the IANA database: https://www.iana.org/assignments/http-dig-alg/http-dig-alg.xhtml +Otherwise the user will get a 403 error for each checksum request they will do. + +Here is a table summarizing the IANA-compliant checksum names: + +| Digest algorithm | HTTP digest | Will be base64 encoded | +|------------------|-------------|------------------------| +| md5 | md5 | true | +| adler32 | adler32 | false | +| sha1 | sha | true | +| sha256 | sha-256 | true | +| sha512 | sha-512 | true | +| cksum | UNIXcksum | false |
I don't think this is what IANA says. UNIXcksum is a 32-bit crc32 checksum. So, the digest algorithm should technically be crc32.. The only mention of cksum is reference to the unix command that computes it. Also, the output should be a decimal number. Was it implemented that way? Note that crc32 returns a hex-string which then needs to be converted to a dec-string.
In src/XrdHttp/README-CKSUM.md:
> @@ -0,0 +1,15 @@ +In the case XrdHttp has been configured, it is important that the configuration contains at least one digest algorithm registered in the IANA database: https://www.iana.org/assignments/http-dig-alg/http-dig-alg.xhtml +Otherwise the user will get a 403 error for each checksum request they will do. + +Here is a table summarizing the IANA-compliant checksum names: + +| Digest algorithm | HTTP digest | Will be base64 encoded | +|------------------|-------------|------------------------| +| md5 | md5 | true | +| adler32 | adler32 | false | +| sha1 | sha | true | +| sha256 | sha-256 | true | +| sha512 | sha-512 | true | +| cksum | UNIXcksum | false | +| crc32 | crc32 | false |
I'm OK with this as we get to say what the value representation is since crc32 is not an IANA registered name.
In src/XrdHttp/XrdHttpChecksum.cc:
> +// XRootD is free software: you can redistribute it and/or modify +// it under the terms of the GNU Lesser General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. +// +// XRootD is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU General Public License for more details. +// +// You should have received a copy of the GNU Lesser General Public License +// along with XRootD. If not, see <http://www.gnu.org/licenses/>. +//------------------------------------------------------------------------------ + +#include "XrdHttpChecksum.hh" +#include <algorithm>
System header first than local headers. Stylistically all local headers are always qualified by the directory they are in so we would says:
#include "XrdHttp/XrdHttpChecksum.hh"
However, I noticed that style was never followed for any code in this directory. So, don't bother changing it. We'll address this as some future point.
In src/XrdHttp/XrdHttpChecksumHandler.cc:
> +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. +// +// XRootD is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU General Public License for more details. +// +// You should have received a copy of the GNU Lesser General Public License +// along with XRootD. If not, see <http://www.gnu.org/licenses/>. +//------------------------------------------------------------------------------ + +#include "XrdHttpChecksumHandler.hh" +#include "XrdOuc/XrdOucTUtils.hh" +#include "XrdOuc/XrdOucUtils.hh" +#include <exception>
System headers ahead of local headers.
In src/XrdHttp/XrdHttpChecksumHandler.cc:
> +
+#include "XrdHttpChecksumHandler.hh"
+#include "XrdOuc/XrdOucTUtils.hh"
+#include "XrdOuc/XrdOucUtils.hh"
+#include <exception>
+#include <algorithm>
+
+std::map<std::string,XrdHttpChecksumHandlerImpl::XrdHttpChecksumPtr> XrdHttpChecksumHandlerImpl::XROOTD_DIGEST_NAME_TO_CKSUMS;
+
+void XrdHttpChecksumHandlerImpl::initializeCksumsMaps() {
+ addChecksumToMaps(std::make_unique<XrdHttpChecksum>("md5","md5",true));
+ addChecksumToMaps(std::make_unique<XrdHttpChecksum>("adler32","adler32",false));
+ addChecksumToMaps(std::make_unique<XrdHttpChecksum>("sha1","sha",true));
+ addChecksumToMaps(std::make_unique<XrdHttpChecksum>("sha256","sha-256",true));
+ addChecksumToMaps(std::make_unique<XrdHttpChecksum>("sha512","sha-512",true));
+ addChecksumToMaps(std::make_unique<XrdHttpChecksum>("cksum","UNIXcksum",false));
See my comment about cksum vs crc32 in a previous note.
In src/XrdHttp/XrdHttpChecksumHandler.hh:
> +// it under the terms of the GNU Lesser General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. +// +// XRootD is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU General Public License for more details. +// +// You should have received a copy of the GNU Lesser General Public License +// along with XRootD. If not, see <http://www.gnu.org/licenses/>. +//------------------------------------------------------------------------------ +#ifndef XROOTD_XRDHTTPCHECKSUMHANDLER_HH +#define XROOTD_XRDHTTPCHECKSUMHANDLER_HH + +#include "XrdHttpChecksum.hh"
Local headers after system headers.
In src/XrdHttp/XrdHttpProtocol.hh:
> @@ -46,6 +46,7 @@ #include "XrdOuc/XrdOucStream.hh" #include "Xrd/XrdProtocol.hh" #include "XrdOuc/XrdOucHash.hh" +#include "XrdHttpChecksumHandler.hh" #include <openssl/ssl.h>
System headers before local headers.
> @@ -43,6 +43,7 @@ #include "XProtocol/XProtocol.hh" #include "XrdXrootd/XrdXrootdBridge.hh" +#include "XrdHttpChecksumHandler.hh" #include <vector>
OK, I get it. the header inversion was there before you started. However, we might as well correct it in the files that git updated. We should make note that it should be corrected everywhere: system header followed by local headers.
> @@ -1400,5 +1400,14 @@ int XrdOucUtils::getModificationTime(const char *path, time_t &modificationTime)
return statRet;
}
+void XrdOucUtils::trim(std::string &str) {
+ // Trim leading non-letters
+ while( str.size() && !isgraph(str[0]) ) str.erase(str.begin());
+
+ // Trim trailing non-letters
+
+ while( str.size() && !isgraph(str[str.size()-1]) )
This is a silly optimization but when you think about it, you want to trim the end first as std:string will not move any characters but merely adjust the size and place a zero at the end. Then when you trim the front there are potentially less characters to move around. I say silly because you likely will never notice this optimization. I couldn't resist though.
In tests/XrdHttpTests/XrdHttpTests.cc:
> @@ -0,0 +1,167 @@ +#undef NDEBUG
So, I am confused. Wasn't the test suite spun off into a separate pull request as @amadio requested? Am seeing something wrong here?
In tests/XrdHttpTests/XrdHttpTests.cc:
> @@ -0,0 +1,167 @@ +#undef NDEBUG + +#include "XrdHttp/XrdHttpReq.hh" +#include "XrdHttp/XrdHttpProtocol.hh" +#include "XrdHttp/XrdHttpChecksumHandler.hh" +#include <exception>
System headers before local headers.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you commented.
Use REPLY-ALL to reply to list
To unsubscribe from the XROOTD-DEV list, click the following link:
https://listserv.slac.stanford.edu/cgi-bin/wa?SUBED1=XROOTD-DEV&A=1