From 5f7e3b72f02e7c70b06d26157cc19451a95f8bcd Mon Sep 17 00:00:00 2001 From: Brian Bockelman Date: Sat, 18 Feb 2023 08:49:21 -0600 Subject: [PATCH 1/3] Add an error object to XrdOss. While XrdOss has a simple POSIX-like interface, in reality the underlying implementation can be quite complicated -- XrdPfc, for example. Accordingly, we should allow for a richer error message interface between the OSS and its callers. This adds a new virtual function which the SFS interface can invoke to get additional error information. The intent is this will be used by XrdPfc and allow clients to differentiate between "the cache gave you a permission denied" and "the origin gave the cache a permission denied". *A note on ABI compatibility*: Technically, this is an ABI change and hence would need to wait until XRootD 6 to be deployed. However, this takes advantage of a quirk of GCC: the ordering of the virtual functions are preserved. Hence, the new functions are placed out-of-order. --- src/XrdOss/XrdOss.cc | 16 ++++++++++++++++ src/XrdOss/XrdOss.hh | 35 +++++++++++++++++++++++++++++++++++ 2 files changed, 51 insertions(+) diff --git a/src/XrdOss/XrdOss.cc b/src/XrdOss/XrdOss.cc index 80b650035e4..0429d4ecb5c 100644 --- a/src/XrdOss/XrdOss.cc +++ b/src/XrdOss/XrdOss.cc @@ -69,6 +69,14 @@ int XrdOss::FSctl(int cmd, int alen, const char *args, char **resp) return -ENOTSUP; } +/******************************************************************************/ +/* g e t E r r o r */ +/******************************************************************************/ +const XrdOucErrInfo *XrdOss::getError() const +{ + return nullptr; +} + /******************************************************************************/ /* R e l o c */ /******************************************************************************/ @@ -153,6 +161,14 @@ int XrdOssDF::Fctl(int cmd, int alen, const char *args, char **resp) return -ENOTSUP; } +/******************************************************************************/ +/* g e t E r r o r */ +/******************************************************************************/ +const XrdOucErrInfo *XrdOssDF::getError() const +{ + return nullptr; +} + /******************************************************************************/ /* p g R e a d */ /******************************************************************************/ diff --git a/src/XrdOss/XrdOss.hh b/src/XrdOss/XrdOss.hh index e1892ade9b5..fd6ffce9507 100644 --- a/src/XrdOss/XrdOss.hh +++ b/src/XrdOss/XrdOss.hh @@ -39,10 +39,12 @@ #include #include +#include "XrdVersion.hh" #include "XrdOss/XrdOssVS.hh" #include "XrdOuc/XrdOucIOVec.hh" class XrdOucEnv; +class XrdOucErrInfo; class XrdSysLogger; class XrdSfsAio; @@ -447,6 +449,21 @@ const char *getTID() {return tident;} virtual ~XrdOssDF() {} +//----------------------------------------------------------------------------- +//! Return advanced error information about the last error encountered by +//! this object. Only invoked if the XrdOSS indicates it has feature XRDOSS_AERR +//! The advanced error feature provides the ability for more error information +//! than is possible from the typical error code (e.g., unstructured error +//! messages). +//! +//! @return Error object associated with the last error. Caller does not own +//! the returned memory. Lifetime of the returned object is only guaranteed +//! until the next method call. May return nullptr even if an error occurred. +//----------------------------------------------------------------------------- + +#if XrdMajorVNUM(XrdVNUMBER) > 5 || defined(__GNUC__) +virtual const XrdOucErrInfo *getError() const; +#endif protected: @@ -479,6 +496,7 @@ short rsvd; // Reserved #define XRDOSS_HASCACH 0x0000000000000010ULL #define XRDOSS_HASNAIO 0x0000000000000020ULL #define XRDOSS_HASRPXY 0x0000000000000040ULL +#define XRDOSS_HASAERR 0x0000000000000080ULL // Options that can be passed to Stat() // @@ -895,6 +913,23 @@ const char *Lfn2Pfn(const char *Path, char *buff, int blen, int &rc) XrdOss() {} virtual ~XrdOss() {} + +//----------------------------------------------------------------------------- +//! Return advanced error information about the last error encountered by +//! this object. Only invoked if the XrdOSS indicates it has feature XRDOSS_AERR +//! The advanced error feature provides the ability for more error information +//! than is possible from the typical error code (e.g., unstructured error +//! messages). +//! +//! @return Error object associated with the last error. Caller does not own +//! the returned memory. Lifetime of the returned object is only guaranteed +//! until the next method call. May return nullptr even if an error occurred. +//----------------------------------------------------------------------------- + +#if XrdMajorVNUM(XrdVNUMBER) > 5 || defined(__GNUC__) +virtual const XrdOucErrInfo *getError() const; +#endif + }; /******************************************************************************/ From 6b88c8369579d7e48e56844d0b5f6d0d94c4fe4f Mon Sep 17 00:00:00 2001 From: Brian Bockelman Date: Sat, 18 Feb 2023 11:22:45 -0600 Subject: [PATCH 2/3] Add advanced error reporting support to the OFS This adds an example of using the new OSS advanced error reporting for an open call. In this case, the underlying error message is concatenated to the standard OFS error message. --- src/XrdOfs/XrdOfs.cc | 40 ++++++++++++++++++++++++++++++++++++- src/XrdOfs/XrdOfs.hh | 3 +++ src/XrdOfs/XrdOfsConfig.cc | 4 ++++ src/XrdOuc/XrdOucErrInfo.hh | 19 +++++++++++++----- 4 files changed, 60 insertions(+), 6 deletions(-) diff --git a/src/XrdOfs/XrdOfs.cc b/src/XrdOfs/XrdOfs.cc index 03c644442b1..8efedbf7362 100644 --- a/src/XrdOfs/XrdOfs.cc +++ b/src/XrdOfs/XrdOfs.cc @@ -737,7 +737,7 @@ int XrdOfsFile::open(const char *path, // In } if (XrdOfsFS->Balancer && retc == -ENOENT) XrdOfsFS->Balancer->Removed(path); - return XrdOfsFS->Emsg(epname, error, retc, "open", path); + return XrdOfsFS->Emsg(epname, error, retc, "open", path, oP.fP); } // Verify that we can actually use this file @@ -2555,6 +2555,44 @@ int XrdOfs::Emsg(const char *pfx, // Message prefix value return SFS_ERROR; } +/******************************************************************************/ + +int XrdOfs::Emsg(const char *pfx, // Message prefix value + XrdOucErrInfo &einfo, // Place to put text & error code + int ecode, // The error code + const char *op, // Operation being performed + const char *target, // The target (e.g., fname) + const XrdOssDF *fP) // The file object generating this error +{ + char buffer[MAXPATHLEN+80]; + +// If the error is EBUSY then we just need to stall the client. This is +// a hack in order to provide for proxy support +// + if (ecode < 0) ecode = -ecode; + if (ecode == EBUSY) return 5; // A hack for proxy support + +// Check for timeout conditions that require a client delay +// + if (ecode == ETIMEDOUT) return OSSDelay; + +// Format the error message +// + XrdOucERoute::Format(buffer, sizeof(buffer), ecode, op, target); + +// Print it out if debugging is enabled +// +#ifndef NODEBUG + OfsEroute.Emsg(pfx, einfo.getErrUser(), buffer); +#endif + +// Place the error message in the error object and return +// + const XrdOucErrInfo *chainedErr = (OssHasErr && fP) ? fP->getError() : nullptr; + einfo.setErrInfo(ecode, buffer, chainedErr); + return SFS_ERROR; +} + /******************************************************************************/ /* P R I V A T E S E C T I O N */ /******************************************************************************/ diff --git a/src/XrdOfs/XrdOfs.hh b/src/XrdOfs/XrdOfs.hh index 5e9fecde0e0..3e18679fbf0 100644 --- a/src/XrdOfs/XrdOfs.hh +++ b/src/XrdOfs/XrdOfs.hh @@ -433,6 +433,8 @@ static int Emsg(const char *, XrdOucErrInfo &, int, const char *x, XrdOfsHandle *hP); static int Emsg(const char *, XrdOucErrInfo &, int, const char *x, const char *y=""); + int Emsg(const char *, XrdOucErrInfo &, int, const char *opName, + const char *path, const XrdOssDF *fP); static int fsError(XrdOucErrInfo &myError, int rc); const char *Split(const char *Args, const char **Opq, char *Path, int Plen); int Stall(XrdOucErrInfo &, int, const char *); @@ -480,6 +482,7 @@ XrdSysMutex ocMutex; // Global mutex for open/close bool DirRdr; // Opendir() can be redirected. bool reProxy; // Reproxying required for TPC bool OssHasPGrw; // True: oss implements full rgRead/Write +bool OssHasErr; // True if the OSS can produce error messages /******************************************************************************/ /* O t h e r D a t a */ diff --git a/src/XrdOfs/XrdOfsConfig.cc b/src/XrdOfs/XrdOfsConfig.cc index 79b5bb7b149..c2c4555fbc5 100644 --- a/src/XrdOfs/XrdOfsConfig.cc +++ b/src/XrdOfs/XrdOfsConfig.cc @@ -357,6 +357,10 @@ int XrdOfs::Configure(XrdSysError &Eroute, XrdOucEnv *EnvInfo) { // OssHasPGrw = (ossFeatures & XRDOSS_HASPGRW) != 0; +// Indicate whether the oss implementation has getError for additional error +// message information. + OssHasErr = (ossFeatures & XRDOSS_HASAERR); + // If POSC processing is enabled (as by default) do it. Warning! This must be // the last item in the configuration list as we need a working filesystem. // Note that in proxy mode we always disable posc! diff --git a/src/XrdOuc/XrdOucErrInfo.hh b/src/XrdOuc/XrdOucErrInfo.hh index 491ba9a1146..5438bc6c860 100644 --- a/src/XrdOuc/XrdOucErrInfo.hh +++ b/src/XrdOuc/XrdOucErrInfo.hh @@ -138,14 +138,23 @@ inline int setErrCode(int code) {return ErrInfo.code = code;} //----------------------------------------------------------------------------- //! Set error code and error text. //! -//! @param code - The error number describing the error. -//! @param emsg - The error message text. +//! @param code - The error number describing the error. +//! @param emsg - The error message text. +//! @param chainedErrInfo - Optionally, an error object triggering the current error //! //! @return code - The error number. //----------------------------------------------------------------------------- -inline int setErrInfo(int code, const char *emsg) - {strlcpy(ErrInfo.message, emsg, sizeof(ErrInfo.message)); +inline int setErrInfo(int code, const char *emsg, const XrdOucErrInfo *chainedErrInfo=nullptr) + {if (chainedErrInfo && chainedErrInfo->getErrText()) + {const char *txtlist[4]; + txtlist[0] = emsg; + txtlist[1] = " ("; + txtlist[2] = chainedErrInfo->getErrText(); + txtlist[3] = ")"; + return setErrInfo(code, txtlist, 4); + } + strlcpy(ErrInfo.message, emsg, sizeof(ErrInfo.message)); if (dataBuff) {dataBuff->Recycle(); dataBuff = 0;} return ErrInfo.code = code; } @@ -259,7 +268,7 @@ inline int getErrInfo(XrdOucEI &errParm) //! @return The pointer to the internal error text. //----------------------------------------------------------------------------- -inline const char *getErrText() +inline const char *getErrText() const {if (dataBuff) return dataBuff->Data(); return (const char *)ErrInfo.message; } From 7712e2b9338d24e365e04b7ee1518c7afc286aa3 Mon Sep 17 00:00:00 2001 From: Brian Bockelman Date: Sat, 18 Feb 2023 11:24:28 -0600 Subject: [PATCH 3/3] Advanced error message generation for the PSS This adds an example of using the new advanced error message generation for the PSS object. With this, opening a file in the cache where the file is not found by the origin will result in something like this: ``` [ERROR] Server responded with an error: [3011] Unable to open /test_access; no such file or directory (Failed to open file in cache: No such file or directory) ``` (line break added for readability) --- src/XrdPss/XrdPss.cc | 10 ++++++++-- src/XrdPss/XrdPss.hh | 6 ++++++ 2 files changed, 14 insertions(+), 2 deletions(-) diff --git a/src/XrdPss/XrdPss.cc b/src/XrdPss/XrdPss.cc index 500a310a6ed..68f4f7430dc 100644 --- a/src/XrdPss/XrdPss.cc +++ b/src/XrdPss/XrdPss.cc @@ -157,7 +157,7 @@ XrdOss *XrdOssGetStorageSystem2(XrdOss *native_oss, XrdPssSys::XrdPssSys() : LocalRoot(0), theN2N(0), DirFlags(0), myVersion(&XrdVERSIONINFOVAR(XrdOssGetStorageSystem2)), - myFeatures(XRDOSS_HASPRXY|XRDOSS_HASPGRW|XRDOSS_HASNOSF) + myFeatures(XRDOSS_HASPRXY|XRDOSS_HASPGRW|XRDOSS_HASNOSF|XRDOSS_HASAERR) {} /******************************************************************************/ @@ -816,7 +816,13 @@ int XrdPssFile::Open(const char *path, int Oflag, mode_t Mode, XrdOucEnv &Env) // Try to open and if we failed, return an error // if (!XrdPssSys::dcaCheck || !ioCache) - {if ((fd = XrdPosixXrootd::Open(pbuff,Oflag,Mode)) < 0) return -errno; + {if ((fd = XrdPosixXrootd::Open(pbuff,Oflag,Mode)) < 0) + {const char *txtlist[2]; + txtlist[0] = "Failed to open file in cache: "; + txtlist[1] = XrdSysError::ec2text(errno); + errInfo.setErrInfo(errno, txtlist, 2); + return -errno; + } } else { XrdPosixInfo Info; Info.ffReady = XrdPssSys::dcaWorld; diff --git a/src/XrdPss/XrdPss.hh b/src/XrdPss/XrdPss.hh index 798312a727e..9dad070dc42 100644 --- a/src/XrdPss/XrdPss.hh +++ b/src/XrdPss/XrdPss.hh @@ -35,6 +35,7 @@ #include #include #include "XrdSys/XrdSysHeaders.hh" +#include "XrdOuc/XrdOucErrInfo.hh" #include "XrdOuc/XrdOucExport.hh" #include "XrdOuc/XrdOucName2Name.hh" #include "XrdOuc/XrdOucPList.hh" @@ -85,6 +86,7 @@ int Fstat(struct stat *); int Fsync(); int Fsync(XrdSfsAio *aiop); int Ftruncate(unsigned long long); +const XrdOucErrInfo *getError() const override {return &errInfo;} ssize_t pgRead (void* buffer, off_t offset, size_t rdlen, uint32_t* csvec, uint64_t opts); int pgRead (XrdSfsAio* aioparm, uint64_t opts); @@ -125,6 +127,8 @@ struct tprInfo char *tpcPath; const XrdSecEntity *entity; + +XrdOucErrInfo errInfo; // Error info object for advanced error reporting. }; /******************************************************************************/ @@ -159,6 +163,7 @@ virtual int Create(const char *, const char *, mode_t, XrdOucEnv &, int opts=0); void EnvInfo(XrdOucEnv *envP); uint64_t Features() {return myFeatures;} +const XrdOucErrInfo *getError() const override {return &errInfo;} int Init(XrdSysLogger *, const char *) override {return -ENOTSUP;} int Init(XrdSysLogger *, const char *, XrdOucEnv *envP) override; int Lfn2Pfn(const char *Path, char *buff, int blen); @@ -216,6 +221,7 @@ unsigned long long DirFlags; // Defaults for exports XrdVersionInfo *myVersion;// -> Compilation version XrdSecsssID *idMapper; // -> Auth ID mapper uint64_t myFeatures;// Our feature set +XrdOucErrInfo errInfo; // Err info object for improved error message reporting int Configure(const char *, XrdOucEnv *); int ConfigProc(const char *ConfigFN);