Print

---

@abh3 requested changes on this pull request.

Several coding style issues (easily correctable) and one significant architectural flaw that is not that easy to fix.

In src/XrdFfs/XrdFfsXrootdfs.cc:

> @@ -442,7 +442,10 @@ static int xrootdfs_create(const char *path, mode_t mode, struct fuse_file_info
     int res, fd;
     if (!S_ISREG(mode))
         return -EPERM;
-    res = xrootdfs_do_create(path, xrootdfs.rdr, O_CREAT | O_WRONLY, true, &fd);
+    if (getenv("XRDCL_EC"))

See my comment in XrdPosixXrootd.cc on how to replace getenv() call with a simple Boolean test.

In src/XrdPosix/XrdPosixXrootd.cc:

> @@ -1060,8 +1060,66 @@ int XrdPosixXrootd::Rename(const char *oldpath, const char *newpath)
 
 // Issue the rename
 //
-  return XrdPosixMap::Result(admin.Xrd.Mv(admin.Url.GetPathWithParams(),
-                             newUrl.GetPathWithParams()));
+   if (! getenv("XRDCL_EC"))

getenv(0 is not cheap and inherently thread-unsafe in the presence of setenv() and it's friends in a threaded environment. I see it used all over the place. So to solve this problem, in namespace XrdPosixGlobals (see the top of the file) define a Boolean, e.g. usingEC, and make sure it's set to true before any calls to XrdPosixXrrotd (typically initialization time).

That, unfortunately, is easier said than done because XrdCl+EC is set by XrdClPlugInManager.cc which, typically but necessarily always, runs after XrdPosix initialization, which makes use of this envar unpredictable and may cause undefined results. I think this whole strategy needs to be rethought on who and when sets this envar/value. Let chat about it since conducting it in this pull request would be overly verbose. In any case, this will not always work and needs to be fixed.

In src/XrdHttp/XrdHttpReq.cc:

> @@ -1367,7 +1367,10 @@ int XrdHttpReq::ProcessHTTPReq() {
         l = resourceplusopaque.length() + 1;
         xrdreq.open.dlen = htonl(l);
         xrdreq.open.mode = htons(kXR_ur | kXR_uw | kXR_gw | kXR_gr | kXR_or);
-        xrdreq.open.options = htons(kXR_mkpath | kXR_open_wrto | kXR_delete);
+        if (! getenv("XRDCL_EC"))

This one is OK as this code path is not often used. However, for consistency you may want to replace it with a simple Boolean test (see XrdPosixXrootd.cc comment).

In src/XrdPosix/XrdPosixXrootd.cc:

> @@ -1060,8 +1060,66 @@ int XrdPosixXrootd::Rename(const char *oldpath, const char *newpath)
 
 // Issue the rename
 //
-  return XrdPosixMap::Result(admin.Xrd.Mv(admin.Url.GetPathWithParams(),
-                             newUrl.GetPathWithParams()));
+   if (! getenv("XRDCL_EC"))
+       return XrdPosixMap::Result(admin.Xrd.Mv(admin.Url.GetPathWithParams(),
+                                  newUrl.GetPathWithParams()));
+   else

This is way too much code to include inline, especially in a naturally short method. Please put it in a separate private method.

In src/XrdPosix/XrdPosixXrootd.cc:

> @@ -1147,7 +1205,7 @@ int XrdPosixXrootd::Stat(const char *path, struct stat *buf)
 
 // Check if we can get the stat informatation from the cache
 //
-  if (XrdPosixGlobals::theCache)
+  if (! getenv("XRDCL_EC") && XrdPosixGlobals::theCache)

Please replace as noted.

In src/XrdPosix/XrdPosixXrootd.cc:

> @@ -1157,7 +1215,107 @@ int XrdPosixXrootd::Stat(const char *path, struct stat *buf)
 
 // Issue the stat and verify that all went well
 //
-   if (!admin.Stat(*buf)) return -1;
+   if (! getenv("XRDCL_EC"))

Please replace as noted.

In src/XrdPosix/XrdPosixXrootd.cc:

> @@ -1157,7 +1215,107 @@ int XrdPosixXrootd::Stat(const char *path, struct stat *buf)
 
 // Issue the stat and verify that all went well
 //
-   if (!admin.Stat(*buf)) return -1;
+   if (! getenv("XRDCL_EC"))
+   {
+       if (!admin.Stat(*buf)) return -1;
+   }
+   else 

Again, way too much code inline in a trivial method. Please move it to a separate private method.

In src/XrdPosix/XrdPosixXrootd.cc:

> @@ -1334,7 +1492,47 @@ int XrdPosixXrootd::Unlink(const char *path)
 
 // Issue the UnLink
 //
-   return XrdPosixMap::Result(admin.Xrd.Rm(admin.Url.GetPathWithParams()));
+   if (! getenv("XRDCL_EC"))

Please replace as noted.

In src/XrdPosix/XrdPosixXrootd.cc:

> @@ -1334,7 +1492,47 @@ int XrdPosixXrootd::Unlink(const char *path)
 
 // Issue the UnLink
 //
-   return XrdPosixMap::Result(admin.Xrd.Rm(admin.Url.GetPathWithParams()));
+   if (! getenv("XRDCL_EC"))
+       return XrdPosixMap::Result(admin.Xrd.Rm(admin.Url.GetPathWithParams()));
+   else

Same story. This needs to go into a separate private method.

In src/XrdTpc/XrdTpcTPC.cc:

> @@ -729,7 +729,7 @@ int TPCHandler::ProcessPullReq(const std::string &resource, XrdHttpExtReq &req)
     XrdSfsFileOpenMode mode = SFS_O_CREAT;
     auto overwrite_header = req.headers.find("Overwrite");
     if ((overwrite_header == req.headers.end()) || (overwrite_header->second == "T")) {
-        mode = SFS_O_TRUNC;
+        if (! getenv("XRDCL_EC")) mode = SFS_O_TRUNC;

This actually does get called often, so it best to replace this with a simple Boolean test as described above.

In src/XrdFfs/XrdFfsXrootdfs.cc:

> @@ -799,6 +802,19 @@ static int xrootdfs_read(const char *path, char *buf, size_t size, off_t offset,
 
     fd = (int) fi->fh;
     XrdFfsWcache_flush(fd);  /* in case is the file is reading/writing */
+
+    if (getenv("XRDCL_EC"))

You may wish to replace this with a simple Boolean test.

In src/XrdApps/Xrdadler32.cc:

> @@ -237,8 +237,14 @@ int main(int argc, char *argv[])
                 printf("Error_accessing: %s\n", argv[1]);
                 return 1;
             }
-            while ( (len = XrdPosixXrootd::Read(fd, buf, N)) > 0 )
-                adler = adler32(adler, (const Bytef*)buf, len);
+            off_t offset = 0;

The name of the variable is totally confusing as it has nothing to do with offset. It has everything to so with st_size which is used to control not running over the end of the file. I'd suggest something like totlen_sofar which is what is being accumulated. A comment of why you need to do this would help.

—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications on the go with GitHub Mobile for iOS or Android.
You are receiving this because you are subscribed to this thread.Message ID: <xrootd/xrootd/pull/1599/review/870075033@github.com>

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