Jeffrey Hutzelman wrote (Wed Jun 17 2009 17:25:00 GMT+0200 (CEST))
--On Wednesday, June 17, 2009 09:29:02 AM -0400 Jeffrey Altman
<[email protected]> wrote:
Felix Frank wrote:
The way I'm currently freeing Capabilities as allocated by the client
stub strikes me as (while conceptionally sound) spectacularly wrong -
how is this supposed to be done?
For a simple case as this (one lone pointer that needs freeing),
osi_free() appears to be the most sensible thing to use after all, as
that is what xdr ultimately does.
Frank:
Call xdr_free() to deallocate memory allocated by the XDR package.
Yes, or better, use XDR in "free" mode, which saves you from having to
know what is supposed to be freed and what isn't. But yeah, just
calling xdr_free() on the ->val thing here should be fine.
I believe that is what the original patch proposal did. However
a) I found no precedent for such behaviour anywhere in any existing code
b) it was real ugly ;/
xdr_free(caps, j * sizeof(Capabilities));
This is a bad example; in the patch in question, caps is the array of
Capabilities structures allocated for rx_multi. It was allocated using
afs_osi_Alloc(), not by XDR, and so should be freed using
afs_osi_Free(), as Frank's patch does.
As announced, it now uses xdr_free for individual Capability_vals and
afs_osi_Free() for the list (the latter being necessary only in
CheckServers(), not afs_GetCapabilities()).
As for the afs_setTime piece. As you and Simon discussed, there is only
one host 'afs_setTimeHost' whose time is used to set the local time.
I believe it is a waste of effort to call GetTime on every host. You
just need to make sure that GetTime is called on 'afs_setTimeHost' if
'afs_setTimeHost' is non-NULL or at least one of the servers such that
afs_IsPrimaryCell(sa->server->cell) is TRUE.
Agree. GetTime should be called only against the set-time host or if
GetCapabilities is unsupported (and really, you can punt the latter case
-- getting RXGEN_OPCODE from GetCapabilities is enough to tell you the
server is up, which is all you really care about).
The idea was to introduce minimum behavior change for the time being,
but you're right of course - if we forego calling GetTime to some
servers, why not go all the way?
Note that all servers are GetTime'd now in the case of a NULL
setTimeHost. Also, server pruning should be correct now (but if GetTime
ends up being completely replaced with GetCapabilities in the setTime ==
0 case, the pruning won't be necessary anyway - only three cases will
remain:
- setTime == 0 -> no GetTime whatsoever
- setTime == 1 && setTimeHost == NULL -> GetTime from all
- else GetTime for setTimeHost only)
Cheers
- Felix
diff --git a/src/afs/afs.h b/src/afs/afs.h
index 4cefbc4..4e21df7 100644
--- a/src/afs/afs.h
+++ b/src/afs/afs.h
@@ -376,6 +376,10 @@ struct srvAddr {
#define SRVR_ISGONE 0x80
#define SNO_INLINEBULK 0x100
#define SNO_64BIT 0x200
+#define SCAPS_KNOWN 0x400
+
+#define SRV_CAPABILITIES(ts) \
+{ if ( ! ts->flags & SCAPS_KNOWN ) afs_GetCapabilities(ts); ts->capabilities; }
#define afs_serverSetNo64Bit(s) ((s)->srvr->server->flags |= SNO_64BIT)
#define afs_serverHasNo64Bit(s) ((s)->srvr->server->flags & SNO_64BIT)
@@ -407,6 +411,7 @@ struct server {
afs_int32 sumOfDowntimes; /* Total downtime experienced, in seconds */
struct srvAddr *addr;
afs_uint32 flags; /* Misc flags */
+ afs_int32 capabilities;
};
#define afs_PutServer(servp, locktype)
diff --git a/src/afs/afs_callback.c b/src/afs/afs_callback.c
index dd003f8..40c2d32 100644
--- a/src/afs/afs_callback.c
+++ b/src/afs/afs_callback.c
@@ -768,9 +768,10 @@ SRXAFSCB_InitCallBackState(struct rx_call *a_call)
ReleaseWriteLock(&afs_xcbhash);
}
}
- }
-
+ /* capabilities need be requested again */
+ ts->flags &= ~SCAPS_KNOWN;
+ }
/* find any volumes residing on this server and flush their state */
{
diff --git a/src/afs/afs_prototypes.h b/src/afs/afs_prototypes.h
index 0f8ceff..f6c17a1 100644
--- a/src/afs/afs_prototypes.h
+++ b/src/afs/afs_prototypes.h
@@ -698,6 +698,7 @@ extern struct server *afs_GetServer(afs_uint32 * aserver, afs_int32 nservers,
afs_int32 acell, u_short aport,
afs_int32 locktype, afsUUID * uuidp,
afs_int32 addr_uniquifier);
+extern void afs_GetCapabilities(struct server *ts);
extern void ForceAllNewConnections(void);
extern void afs_MarkServerUpOrDown(struct srvAddr *sa, int a_isDown);
extern afs_int32 afs_ServerDown(struct srvAddr *sa);
diff --git a/src/afs/afs_server.c b/src/afs/afs_server.c
index 3e683c3..8097038 100644
--- a/src/afs/afs_server.c
+++ b/src/afs/afs_server.c
@@ -532,7 +532,7 @@ afs_CheckServers(int adown, struct cell *acellp)
struct server *ts;
struct srvAddr *sa;
struct conn *tc;
- afs_int32 i, j;
+ afs_int32 i, j, k;
afs_int32 code;
afs_int32 start, end = 0, delta;
osi_timeval_t tv;
@@ -541,9 +541,10 @@ afs_CheckServers(int adown, struct cell *acellp)
int srvAddrCount;
struct srvAddr **addrs;
struct conn **conns;
- int nconns;
+ int nconns, safe;
struct rx_connection **rxconns;
afs_int32 *conntimer, *deltas, *results;
+ Capabilities *caps = NULL;
AFS_STATCNT(afs_CheckServers);
@@ -583,6 +584,9 @@ afs_CheckServers(int adown, struct cell *acellp)
deltas = (afs_int32 *)afs_osi_Alloc(j * sizeof (afs_int32));
results = (afs_int32 *)afs_osi_Alloc(j * sizeof (afs_int32));
+ caps = (Capabilities *)afs_osi_Alloc(j * sizeof (Capabilities));
+ memset(caps, 0, j * sizeof(Capabilities));
+
for (i = 0; i < j; i++) {
deltas[i] = 0;
sa = addrs[i];
@@ -631,6 +635,60 @@ afs_CheckServers(int adown, struct cell *acellp)
}
} /* Outer loop over addrs */
+ AFS_GUNLOCK();
+ multi_Rx(rxconns,nconns)
+ {
+ multi_RXAFS_GetCapabilities(&caps[multi_i]);
+ results[multi_i] = multi_error;
+ } multi_End;
+ AFS_GLOCK();
+
+ for ( i = 0 ; i < nconns ; i++ ) {
+ ts = addrs[i]->server;
+ if ( !ts )
+ continue;
+ ts->capabilities = 0;
+ ts->flags |= SCAPS_KNOWN;
+ if ( results[i] == RXGEN_OPCODE )
+ continue;
+ if ( results[i] >= 0 )
+ /* we currently handle 32-bits of capabilities */
+ if (caps[i].Capabilities_len > 0) {
+ ts->capabilities = caps[i].Capabilities_val[0];
+ xdr_free(caps[i].Capabilities_val,
+ caps[i].Capabilities_len * sizeof(afs_int32));
+ caps[i].Capabilities_val = NULL;
+ caps[i].Capabilities_len = 0;
+ }
+ }
+
+ safe = nconns;
+
+ /* prune the servers that sent capabilities */
+ for ( i=0, k=0; i<nconns; i++) {
+ /* be sure to include the setTimeHost if necessary */
+ if (results[i] == RXGEN_OPCODE ||
+ (afs_setTime && afs_setTimeHost == NULL) ||
+ (afs_setTime && conns[i]->srvr->server == afs_setTimeHost)) {
+ if (i != k) { /*swap incapable servers to beginning of list*/
+#define SWAP(array,tmp) tmp=array[k]; array[k]=array[i]; array[i]=tmp;
+ void *pointer_bucket;
+ afs_int32 int_bucket;
+ SWAP(conns, pointer_bucket);
+ SWAP(rxconns, pointer_bucket);
+ SWAP(conntimer, int_bucket);
+ SWAP(deltas, int_bucket);
+#undef SWAP
+ }
+ k++;
+ }
+ else if ( !results[i] ) {
+ afs_PutConn(conns[i], SHARED_LOCK);
+ }
+ }
+ nconns = k; /* next, only use servers that couldn't GetCapabilities */
+
+ if ( nconns ) {
start = osi_Time(); /* time the gettimeofday call */
AFS_GUNLOCK();
multi_Rx(rxconns,nconns)
@@ -648,6 +706,9 @@ afs_CheckServers(int adown, struct cell *acellp)
} multi_End;
AFS_GLOCK();
+ }
+
+ nconns = safe; /* restore in case of GetTime limiting */
for(i=0;i<nconns;i++){
tc = conns[i];
@@ -666,7 +727,8 @@ afs_CheckServers(int adown, struct cell *acellp)
if (afs_waitForeverCount) {
afs_osi_Wakeup(&afs_waitForever);
}
- } else {
+ }
+ else {
if (results[i] < 0) {
/* server crashed */
afs_ServerDown(sa);
@@ -750,6 +812,7 @@ afs_CheckServers(int adown, struct cell *acellp)
afs_osi_Free(conntimer, j * sizeof(afs_int32));
afs_osi_Free(deltas, j * sizeof(afs_int32));
afs_osi_Free(results, j * sizeof(afs_int32));
+ afs_osi_Free(caps, j * sizeof(Capabilities));
} /*afs_CheckServers*/
@@ -1616,6 +1679,48 @@ void afs_RemoveSrvAddr(struct srvAddr *sap) {
}
}
+/* afs_GetCapabilities
+ * Try and retrieve capabilities of a given file server. Carps on actual
+ * failure. Servers are not expected to support this RPC. */
+void afs_GetCapabilities(struct server *ts)
+{
+ Capabilities caps = {0, NULL};
+ struct vrequest treq;
+ struct conn *tc;
+ struct unixuser *tu;
+ afs_int32 code;
+
+ if ( !ts )
+ return;
+ if ( !afs_osi_credp )
+ return;
+
+ if ((code = afs_InitReq(&treq, afs_osi_credp)))
+ return;
+ tu = afs_GetUser(treq.uid, ts->cell->cellNum, SHARED_LOCK);
+ if ( !tu )
+ return;
+ tc = afs_ConnBySA(ts->addr, ts->cell->fsport, ts->cell->cellNum, tu, 0, 1,
+ SHARED_LOCK);
+ if ( !tc )
+ return;
+ code = RXAFS_GetCapabilities(tc->id, &caps);
+ if ( code && code != RXGEN_OPCODE )
+ afs_warn("RXAFS_GetCapabilities failed with code %d\n", code);
+ else
+ ts->flags |= SCAPS_KNOWN;
+ afs_PutConn(tc, SHARED_LOCK);
+
+ if ( caps.Capabilities_len > 0 ) {
+ ts->capabilities = caps.Capabilities_val[0];
+ xdr_free(caps.Capabilities_val,
+ caps.Capabilities_len * sizeof(afs_int32));
+ caps.Capabilities_len = 0;
+ caps.Capabilities_val = NULL;
+ }
+
+}
+
/* afs_GetServer()
* Return an updated and properly initialized server structure
* corresponding to the server ID, cell, and port specified.
@@ -1625,7 +1730,8 @@ void afs_RemoveSrvAddr(struct srvAddr *sap) {
struct server *afs_GetServer(afs_uint32 * aserverp, afs_int32 nservers,
afs_int32 acell, u_short aport,
afs_int32 locktype, afsUUID * uuidp,
- afs_int32 addr_uniquifier) {
+ afs_int32 addr_uniquifier)
+{
struct server *oldts = 0, *ts, *newts, *orphts = 0;
struct srvAddr *oldsa, *newsa, *nextsa, *orphsa;
u_short fsport;
@@ -1819,6 +1925,9 @@ struct server *afs_GetServer(afs_uint32 * aserverp, afs_int32 nservers,
afs_stats_cmperf.srvRecordsHWM = afs_stats_cmperf.srvRecords;
}
+ if ( aport == AFS_FSPORT && !(newts->flags & SCAPS_KNOWN))
+ afs_GetCapabilities(newts);
+
ReleaseWriteLock(&afs_xsrvAddr);
ReleaseWriteLock(&afs_xserver);
return (newts);
diff --git a/src/libafsrpc/afsrpc.def b/src/libafsrpc/afsrpc.def
index cd75567..3c0f173 100644
--- a/src/libafsrpc/afsrpc.def
+++ b/src/libafsrpc/afsrpc.def
@@ -217,5 +217,6 @@ EXPORTS
rxkad_stats_key @222 DATA
rx_InitHost @224
rx_NewServiceHost @225
+ xdr_free @252
diff --git a/src/rx/xdr.c b/src/rx/xdr.c
index 1a01bab..3de1467 100644
--- a/src/rx/xdr.c
+++ b/src/rx/xdr.c
@@ -583,4 +583,10 @@ xdr_wrapstring(register XDR * xdrs, char **cpp)
return (FALSE);
}
#endif
+
+void
+xdr_free(void *x, afs_int32 size)
+{
+ osi_free(x, size);
+}
#endif /* NeXT */
diff --git a/src/rx/xdr_prototypes.h b/src/rx/xdr_prototypes.h
index 5c1898e..e36ddd4 100644
--- a/src/rx/xdr_prototypes.h
+++ b/src/rx/xdr_prototypes.h
@@ -58,6 +58,7 @@ extern bool_t xdr_union(register XDR * xdrs, enum_t * dscmp, caddr_t unp,
struct xdr_discrim *choices, xdrproc_t dfault);
extern bool_t xdr_string(register XDR * xdrs, char **cpp, u_int maxsize);
extern bool_t xdr_wrapstring(register XDR * xdrs, char **cpp);
+extern void xdr_free(void *x, afs_int32 size);
/* xdr_float.c */
