Callers of rx_EndCall in various parts of the code handle errors a bit
differently from each other. The correct way to use rx_EndCall is
almost always some form of:
code = rx_EndCall(call, code);
This will cause the call to abort with 'code' if the call is not
already aborted, and will return the abort code for the call (or 0 if
the call ended successfully). It is thus impossible for 'code' to
start out with a non-zero value in the code snippet above, and end up
with a value of 0 after the code snippet.
Most code follows this pattern, because this is how the
rxgen-generated client RPC wrappers are written. So for any non-split
Rx call, this is how the error precedence works.
However, some code (mostly for Rx split calls), needs to handle
calling rx_EndCall itself, and some code appears to think it is
possible for rx_EndCall to return 0 when we already had a non-zero
error. Such code tries to ensure that we don't ignore an error we
already got by doing something like this:
code2 = rx_EndCall(call, code);
if (code2 && !code) {
code = code2;
}
However, this is not correct. If a call gets killed with an abort code
partway through executing an RPC, and the client tries to end the RPC
with e.g. EndRXAFS_FetchData, the client will get an error code of
-451 (RXGEN_CC_UNMARSHAL). The actual error code is in the abort code
for the call, but with the above 'code2' snippet, we can easily return
an error of -451 instead, which will usually get interpreted as some
unknown network-related error.
This can manifest as a problem in the unix client, where if a
FetchData call fails due to, for example, an "idle dead" timeout, we
should result with an error code of RX_CALL_TIMEOUT. But because of
the above issue, we'll instead yield an error of -451, causing the
server to be marked down with the following message:
afs: Lost contact with file server ... (code -451) ...
So, fix most rx_EndCall callers to follow the 'code = rx_EndCall(call,
code);' pattern. Not all of the changes here are to "wrong" code, but
try to make all of the rx_EndCall call sites look more consistent.
There are a few exceptions to this pattern, which warrant some
variations:
- A few instances in src/WINNT/afsd/cm_dcache.c do seem to want to
record the original error before we ran rx_EndCall, instead of
seeing the rx abort code. We still return the rx_EndCall-returned
value to the caller, though.
- Any caller of RXAFS_FetchData* needs to read a 'length' raw from
the rx split stream. If this fails, we need to abort the call, but
we don't really have an error code to give to rx_EndCall. Failure
to read a length indicates that the server is not following
protocol properly, so give rx_EndCall RX_PROTOCOL_ERROR in these
instances. The call should already be aborted by this point, so
most of the time this code will be ignored; it will only make a
difference if the server tries to end the call successfully without
sending a length, which is indeed a protocol error.
- Some Rx clients can encounter a local error they don't want to send
to the server via an abort, so they just end the call successfully,
and only use the rx abort code if they don't already have a local
error. This is in a few places like src/butc/dump.c and
src/volser/vsprocs.c.
- Several places don't care what the error from rx_EndCall is, such
as various call sites in server-side code.
The behavior of the Windows client w.r.t rx_EndCall was changed a bit
into its current behavior in commit
a50fa631cad6919d15721ac2c234ebbdda2b4031 (ticket 125018), which just
appears to be wrong. This was partially reverted by commit
ae7ef5f5b963a5c8ce4110a7352e0010cb6cdbc1 (ticket 125351), but some of
the other call sites were unchanged. The Unix client appears to have
been doing this incorrectly for at least FetchData calls since OpenAFS
1.0.
To make it hopefully more clear that rx_EndCall cannot return 0 if
given a non-zero error code, add an assert to rx_EndCall that asserts
that fact.
FIXES 127001
Change-Id: I10bbfe82b55b509e1930abb6c568edb1efd9fd2f
Reviewed-on: http://gerrit.openafs.org/10788
Reviewed-by: D Brashear <shadow@your-file-system.com>
Tested-by: BuildBot <buildbot@rampaginggeek.com>
* but the vnode involved may or may not be locked depending on whether
* or not the CM_BUF_WRITE_SCP_LOCKED flag is set.
*/
- long code, code1;
+ long code;
cm_scache_t *scp = vscp;
afs_int32 nbytes;
afs_int32 save_nbytes;
}
}
- code1 = rx_EndCall(rxcallp, code);
+ /* Prefer rx_EndCall error over StoreData error. Note that this will
+ * never set 'code' to 0 if we passed in a non-zero code. */
+ code = rx_EndCall(rxcallp, code);
- if ((code == RXGEN_OPCODE || code1 == RXGEN_OPCODE) && SERVERHAS64BIT(connp)) {
+ if (code == RXGEN_OPCODE && SERVERHAS64BIT(connp)) {
SET_SERVERHASNO64BIT(connp);
qdp = NULL;
nbytes = save_nbytes;
goto retry;
}
- /* Prefer rx_EndCall error over StoreData error */
- if (code1 != 0) {
- osi_Log2(afsd_logp, "rx_EndCall converted 0x%x to 0x%x", code, code1);
- code = code1;
- }
} while (cm_Analyze(connp, userp, reqp, &scp->fid, NULL, 1, &outStatus, &volSync, NULL, NULL, code));
code = cm_MapRPCError(code, reqp);
AFSStoreStatus inStatus;
AFSVolSync volSync;
AFSFid tfid;
- long code, code1;
+ long code;
osi_hyper_t truncPos;
cm_conn_t *connp;
struct rx_call *rxcallp;
osi_Log0(afsd_logp, "EndRXAFS_StoreData SUCCESS");
}
}
- code1 = rx_EndCall(rxcallp, code);
+ code = rx_EndCall(rxcallp, code);
- if ((code == RXGEN_OPCODE || code1 == RXGEN_OPCODE) && SERVERHAS64BIT(connp)) {
+ if (code == RXGEN_OPCODE && SERVERHAS64BIT(connp)) {
SET_SERVERHASNO64BIT(connp);
goto retry;
}
-
- /* prefer StoreData error over rx_EndCall error */
- if (code == 0 && code1 != 0)
- code = code1;
} while (cm_Analyze(connp, userp, reqp, &scp->fid, NULL, 1, &outStatus, &volSync, NULL, NULL, code));
code = cm_MapRPCError(code, reqp);
long cm_GetBuffer(cm_scache_t *scp, cm_buf_t *bufp, int *cpffp, cm_user_t *userp,
cm_req_t *reqp)
{
- long code=0, code1=0;
+ long code=0;
afs_uint32 nbytes; /* bytes in transfer */
afs_uint32 nbytes_hi = 0; /* high-order 32 bits of bytes in transfer */
afs_uint64 length_found = 0;
/* now make the call */
do {
+ long code1;
+
code = cm_ConnFromFID(&scp->fid, userp, reqp, &connp);
if (code)
continue;
nbytes_hi = ntohl(nbytes_hi);
} else {
nbytes_hi = 0;
- code = rx_Error(rxcallp);
- code1 = rx_EndCall(rxcallp, code);
+ code = rx_EndCall(rxcallp, RX_PROTOCOL_ERROR);
rxcallp = NULL;
}
}
osi_Log1(afsd_logp, "CALL EndRXAFS_FetchData skipped due to error %d", code);
}
+ code1 = code;
if (rxcallp)
code1 = rx_EndCall(rxcallp, code);
scp_locked = 0;
}
code = 0;
- /* Prefer the error value from FetchData over rx_EndCall */
- } else if (code == 0 && code1 != 0)
+ } else {
+ /* Prefer the error from rx_EndCall over any other error */
code = code1;
+ }
osi_Log0(afsd_logp, "CALL FetchData DONE");
} while (cm_Analyze(connp, userp, reqp, &scp->fid, NULL, 0, &afsStatus, &volSync, NULL, NULL, code));
long cm_GetData(cm_scache_t *scp, osi_hyper_t *offsetp, char *datap, int data_length,
int * bytes_readp, cm_user_t *userp, cm_req_t *reqp)
{
- long code=0, code1=0;
+ long code=0;
afs_uint32 nbytes; /* bytes in transfer */
afs_uint32 nbytes_hi = 0; /* high-order 32 bits of bytes in transfer */
afs_uint64 length_found = 0;
/* now make the call */
do {
+ long code1;
+
code = cm_ConnFromFID(&scp->fid, userp, reqp, &connp);
if (code)
continue;
nbytes_hi = ntohl(nbytes_hi);
} else {
nbytes_hi = 0;
- code = rx_Error(rxcallp);
- code1 = rx_EndCall(rxcallp, code);
+ code = rx_EndCall(rxcallp, RX_PROTOCOL_ERROR);
rxcallp = NULL;
}
}
osi_Log1(afsd_logp, "CALL EndRXAFS_FetchData skipped due to error %d", code);
}
+ code1 = code;
if (rxcallp)
code1 = rx_EndCall(rxcallp, code);
scp_locked = 0;
}
code = 0;
- /* Prefer the error value from FetchData over rx_EndCall */
- } else if (code == 0 && code1 != 0)
+ } else {
+ /* Prefer the error from rx_EndCall over any other error */
code = code1;
+ }
osi_Log0(afsd_logp, "CALL FetchData DONE");
} while (cm_Analyze(connp, userp, reqp, &scp->fid, NULL, 0, &afsStatus, &volSync, NULL, NULL, code));
long
cm_VerifyStoreData(cm_bulkIO_t *biod, cm_scache_t *savedScp)
{
- long code=0, code1=0;
+ long code=0;
afs_uint32 nbytes; /* bytes in transfer */
afs_uint32 nbytes_hi = 0; /* high-order 32 bits of bytes in transfer */
afs_uint64 length_found = 0;
/* now make the call */
do {
+ long code1;
+
code = cm_ConnFromFID(&scp->fid, userp, reqp, &connp);
if (code)
continue;
nbytes_hi = ntohl(nbytes_hi);
} else {
nbytes_hi = 0;
- code = rx_Error(rxcallp);
- code1 = rx_EndCall(rxcallp, code);
+ code = rx_EndCall(rxcallp, RX_PROTOCOL_ERROR);
rxcallp = NULL;
}
}
osi_Log1(afsd_logp, "CALL EndRXAFS_FetchData skipped due to error %d", code);
}
+ code1 = code;
if (rxcallp)
code1 = rx_EndCall(rxcallp, code);
scp_locked = 0;
}
code = 0;
- /* Prefer the error value from FetchData over rx_EndCall */
- } else if (code == 0 && code1 != 0)
+ } else {
+ /* Prefer the error from rx_EndCall over any other error */
code = code1;
+ }
osi_Log0(afsd_logp, "CALL FetchData DONE");
} while (cm_Analyze(connp, userp, reqp, &scp->fid, NULL, 0, &afsStatus, &volSync, NULL, NULL, code));
IN void *memoryRegionp,
OUT afs_uint32 *bytesWritten)
{
- long code, code1;
+ long code;
long temp;
AFSFetchStatus outStatus;
AFSStoreStatus inStatus;
}
}
- code1 = rx_EndCall(rxcallp, code);
+ code = rx_EndCall(rxcallp, code);
- if ((code == RXGEN_OPCODE || code1 == RXGEN_OPCODE) && SERVERHAS64BIT(connp)) {
+ if (code == RXGEN_OPCODE && SERVERHAS64BIT(connp)) {
SET_SERVERHASNO64BIT(connp);
goto retry;
}
-
- /* Prefer StoreData error over rx_EndCall error */
- if (code1 != 0)
- code = code1;
} while (cm_Analyze(connp, userp, reqp, &scp->fid, NULL, 1, &outStatus, &volSync, NULL, NULL, code));
code = cm_MapRPCError(code, reqp);
if (bytes != sizeof(afs_int32)) {
length_hi = 0;
- code = rx_Error(tcall);
COND_GUNLOCK(locked);
- code = rx_EndCall(tcall, code);
+ code = rx_EndCall(tcall, RX_PROTOCOL_ERROR);
COND_RE_GLOCK(locked);
tcall = NULL;
}
}
afs_int32
-rxfs_storeDestroy(void **r, afs_int32 error)
+rxfs_storeDestroy(void **r, afs_int32 code)
{
- afs_int32 code = error;
struct rxfs_storeVariables *v = (struct rxfs_storeVariables *)*r;
*r = NULL;
if (v->call) {
RX_AFS_GUNLOCK();
- code = rx_EndCall(v->call, error);
+ code = rx_EndCall(v->call, code);
RX_AFS_GLOCK();
- if (!code && error)
- code = error;
}
if (v->tbuffer)
osi_FreeLargeSpace(v->tbuffer);
rxfs_fetchClose(void *r, struct vcache *avc, struct dcache * adc,
struct afs_FetchOutput *o)
{
- afs_int32 code, code1 = 0;
+ afs_int32 code;
struct rxfs_fetchVariables *v = (struct rxfs_fetchVariables *)r;
if (!v->call)
#endif
code = EndRXAFS_FetchData(v->call, &o->OutStatus, &o->CallBack,
&o->tsync);
- code1 = rx_EndCall(v->call, code);
+ code = rx_EndCall(v->call, code);
RX_AFS_GLOCK();
- if (!code && code1)
- code = code1;
v->call = NULL;
}
afs_int32
-rxfs_fetchDestroy(void **r, afs_int32 error)
+rxfs_fetchDestroy(void **r, afs_int32 code)
{
- afs_int32 code = error;
struct rxfs_fetchVariables *v = (struct rxfs_fetchVariables *)*r;
*r = NULL;
if (v->call) {
RX_AFS_GUNLOCK();
- code = rx_EndCall(v->call, error);
+ code = rx_EndCall(v->call, code);
RX_AFS_GLOCK();
- if (error)
- code = error;
}
if (v->tbuffer)
osi_FreeLargeSpace(v->tbuffer);
struct osi_file *fP, struct fetchOps **ops, void **rock)
{
struct rxfs_fetchVariables *v;
- int code = 0, code1 = 0;
+ int code = 0;
#ifdef AFS_64BIT_CLIENT
afs_uint32 length_hi = 0;
#endif
if (bytes == sizeof(afs_int32)) {
length_hi = ntohl(length_hi);
} else {
- code = rx_Error(v->call);
RX_AFS_GUNLOCK();
- code1 = rx_EndCall(v->call, code);
+ code = rx_EndCall(v->call, RX_PROTOCOL_ERROR);
RX_AFS_GLOCK();
v->call = NULL;
}
length = ntohl(length);
else {
RX_AFS_GUNLOCK();
- code = rx_Error(v->call);
- code1 = rx_EndCall(v->call, code);
+ code = rx_EndCall(v->call, RX_PROTOCOL_ERROR);
v->call = NULL;
length = 0;
RX_AFS_GLOCK();
if (bytes == sizeof(afs_int32)) {
*alength = ntohl(length);
} else {
- code = rx_Error(v->call);
- code1 = rx_EndCall(v->call, code);
+ code = rx_EndCall(v->call, RX_PROTOCOL_ERROR);
v->call = NULL;
}
}
* requested. It shouldn't do that, and accepting that much data
* can make us take up more cache space than we're supposed to,
* so error. */
- code = rx_Error(v->call);
RX_AFS_GUNLOCK();
- code1 = rx_EndCall(v->call, code);
+ code = rx_EndCall(v->call, RX_PROTOCOL_ERROR);
RX_AFS_GLOCK();
v->call = NULL;
code = EIO;
}
- if (!code && code1)
- code = code1;
-
if (code) {
osi_FreeSmallSpace(v);
return code;
code =
StartBOZO_GetLog(tcall, AFSDIR_CANONICAL_SERVER_SLVGLOG_FILEPATH);
if (code) {
- rx_EndCall(tcall, code);
+ code = rx_EndCall(tcall, code);
goto done;
}
/* copy data */
tcall = rx_NewCall(tconn);
code = StartBOZO_GetLog(tcall, as->parms[1].items->data);
if (code) {
- rx_EndCall(tcall, code);
+ code = rx_EndCall(tcall, code);
goto done;
}
/* copy data */
(afs_int32) estat.st_mode, estat.st_mtime);
if (tst) {
- rx_EndCall(tcall, tst);
+ tst = rx_EndCall(tcall, tst);
goto fail_bos_ExecutableCreate;
}
fail_bos_LogGet:
if (have_call) {
- rx_EndCall(tcall, 0);
+ tst = rx_EndCall(tcall, 0);
}
if (st != NULL) {
struct rx_connection *fromconn;
struct rx_call *fromcall;
afs_int32 fromtid;
- afs_int32 rxError;
afs_int32 rcode;
struct nvldbentry entry;
int islocked;
islocked = 0;
- rxError = 0;
fromconn = (struct rx_connection *)0;
fromtid = 0;
fromcall = (struct rx_call *)0;
if ((tst = DumpFunction(fromcall, filename))) {
goto fail_UV_DumpVolume;
}
- tst = rx_EndCall(fromcall, rxError);
+ tst = rx_EndCall(fromcall, 0);
fromcall = (struct rx_call *)0;
if (tst) {
goto fail_UV_DumpVolume;
}
if (fromcall) {
- etst = rx_EndCall(fromcall, rxError);
+ etst = rx_EndCall(fromcall, 0);
if (etst) {
if (!tst)
tst = etst;
struct rx_connection *toconn, *tempconn;
struct rx_call *tocall;
afs_int32 totid, rcode;
- afs_int32 rxError = 0;
struct volser_status tstatus;
char partName[10];
afs_uint32 pvolid;
if (tst) {
goto fail_UV_RestoreVolume;
}
- tst = rx_EndCall(tocall, rxError);
+ tst = rx_EndCall(tocall, 0);
tocall = (struct rx_call *)0;
if (tst) {
goto fail_UV_RestoreVolume;
fail_UV_RestoreVolume:
if (tocall) {
- etst = rx_EndCall(tocall, rxError);
+ etst = rx_EndCall(tocall, 0);
if (!tst)
tst = etst;
}
struct afscp_volume *vol;
struct afscp_server *server;
struct rx_call *c = NULL;
- int code, code2 = 0;
+ int code;
int i, j, bytes, totalbytes = 0;
int bytesremaining;
char *p;
rx_Read(c, (char *)&bytesremaining,
sizeof(afs_int32));
if (bytes != sizeof(afs_int32)) {
- code = rx_EndCall(c, bytes);
+ code = rx_EndCall(c, RX_PROTOCOL_ERROR);
continue;
}
bytesremaining = ntohl(bytesremaining);
}
if (bytesremaining == 0) {
time(&now);
- code2 = EndRXAFS_FetchData(c, &fst, &cb, &vs);
- if (code2 == 0)
+ code = EndRXAFS_FetchData(c, &fst, &cb, &vs);
+ if (code == 0)
afscp_AddCallBack(server, &fid->fid, &fst, &cb,
now);
}
- code = rx_EndCall(c, code2);
+ code = rx_EndCall(c, code);
}
if (code == 0) {
return totalbytes;
struct afscp_volume *vol;
struct afscp_server *server;
struct rx_call *c = NULL;
- int code, code2 = 0;
+ int code;
int i, j, bytes, totalbytes = 0;
int bytesremaining;
const char *p;
bytesremaining -= bytes;
}
if (bytesremaining == 0) {
- code2 = EndRXAFS_StoreData(c, &fst, &vs);
+ code = EndRXAFS_StoreData(c, &fst, &vs);
}
- code = rx_EndCall(c, code2);
+ code = rx_EndCall(c, code);
}
if (code == 0) {
return totalbytes;
* Map errors to the local host's errno.h format.
*/
error = ntoh_syserr_conv(error);
+
+ /* If the caller said the call failed with some error, we had better
+ * return an error code. */
+ osi_Assert(!rc || error);
return error;
}
void *
urecovery_Interact(void *dummy)
{
- afs_int32 code, tcode;
+ afs_int32 code;
struct ubik_server *bestServer = NULL;
struct ubik_server *ts;
int dbok, doingRPC, now;
goto FetchEndCall;
code = EndDISK_GetFile(rxcall, &tversion);
FetchEndCall:
- tcode = rx_EndCall(rxcall, code);
- if (!code)
- code = tcode;
+ code = rx_EndCall(rxcall, code);
if (!code) {
/* we got a new file, set up its header */
urecovery_state |= UBIK_RECHAVEDB;
struct rx_connection * volatile fromconn = NULL;
afs_int32 volatile fromtid = 0;
- afs_int32 rxError = 0, rcode = 0;
+ afs_int32 rcode = 0;
afs_int32 code, error = 0;
afs_int32 tmp;
time_t tmv = fromdate;
error_exit:
if (fromcall) {
- code = rx_EndCall(fromcall, rxError);
+ code = rx_EndCall(fromcall, 0);
if (code && code != RXGEN_OPCODE)
fprintf(STDERR, "Error in rx_EndCall\n");
if (code && !error)
afs_uint32 volatile clonevol = 0;
afs_int32 tmp;
- afs_int32 fromtid = 0, rxError = 0, rcode = 0;
+ afs_int32 fromtid = 0, rcode = 0;
afs_int32 code = 0, error = 0;
afs_uint32 tmpVol;
char vname[64];
}
if (fromcall) {
- code = rx_EndCall(fromcall, rxError);
+ code = rx_EndCall(fromcall, 0);
if (code) {
fprintf(STDERR, "Error in rx_EndCall\n");
if (!error)
struct rx_connection *toconn, *tempconn;
struct rx_call *tocall;
afs_int32 totid, code, rcode, vcode, terror = 0;
- afs_int32 rxError = 0;
struct volser_status tstatus;
struct volintInfo vinfo;
char partName[10];
error = code;
goto refail;
}
- terror = rx_EndCall(tocall, rxError);
+ terror = rx_EndCall(tocall, 0);
tocall = (struct rx_call *)0;
if (terror) {
fprintf(STDERR, "rx_EndCall Failed \n");
}
refail:
if (tocall) {
- code = rx_EndCall(tocall, rxError);
+ code = rx_EndCall(tocall, 0);
if (!error)
error = code;
}