PelicanPlatform / xrootd-s3-http

An XRootD plugin that allows Pelican to interface with s3/http server backends
Apache License 2.0
2 stars 6 forks source link

Simple cleanups of the S3/HTTP backend code #21

Closed bbockelm closed 8 months ago

bbockelm commented 8 months ago

Includes:

bbockelm commented 8 months ago

@jhiemstrawisc - do you know what's up with the linter here?

I mean, it does reasonable suggestions ... just its suggestions are so wide-ranging that one would lose sight of what the real changes are. I'd suggest that we merge this (and #22 if ready) and then do a codebase-wide reformat.

(I do like the suggested reformatting changes, it's just the original stylings are quite different)

jhiemstrawisc commented 8 months ago

RE the linter, I think I started to add a few CI/CD pieces to this repo awhile ago, but hit a time crunch elsewhere that prevented me from getting it all in working order. I like your plan of merging #21 and #22 and then doing a repo-wide refactor.

jhiemstrawisc commented 8 months ago

@bbockelm It looks like our operating systems disagree about the underlying type of off_t. I see you changed various statements from %zu to %lld, and this breaks things for me on Linux:

/workspaces/xrootd-s3-dev/xrootd-s3-http/src/S3Commands.cc: In member function ‘virtual bool AmazonS3Upload::SendRequest(const string&, off_t, size_t)’:
/workspaces/xrootd-s3-dev/xrootd-s3-http/src/S3Commands.cc:400:20: error: format ‘%lld’ expects argument of type ‘long long int’, but argument 3 has type ‘off_t’ {aka ‘long int’} [-Werror=format=]
   formatstr(range, "bytes=%lld-%lld", offset, offset + size - 1);

Do you want me to take this over and worry about portability, or is that something you want to handle? I'm no multi-platform C++ guru, but I'm happy to give things a shot.

bbockelm commented 8 months ago

@jhiemstrawisc - can you take it over?

For this particular item: I think you can simply cast the offset to a long long int (and then use %llu in the format specifier) to avoid potential issues as that's going to always be the larger of the two and not risk truncation issues.

Note that on 32-bit systems (which are near-impossible to find these days...), it's possible off_t is 32-bit if it is a long (as many 32-bit environments used a 32-bit long) unless a certain flag was set when compiling. This was a big headache about 20 years ago as it required a transition to use files greater than ~2GB.