Commit de46edef69 (Remove lowercase min/max macro definitions)
inadvertently removed a definition of max() that is required by
_krb5_n_fold() (src/external/heimdal/krb5/n-fold.c) if no system
definition of max() is available. For example, on Solaris this results
in a kernel module with an undefined external symbol 'max', which
prevents the kernel module from loading:
genunix: [ID 819705 kern.notice] /kernel/drv/amd64/afs: undefined symbol
genunix: [ID 826211 kern.notice] 'max'
genunix: [ID 472681 kern.notice] WARNING: mod_load: cannot load module 'afs'
Restore the required max() macro definition.
Change-Id: I6bca2bb2b90d7cdbe90a3b769997cdc153f59f6c
Reviewed-on: https://gerrit.openafs.org/15874
Tested-by: BuildBot <buildbot@rampaginggeek.com>
Reviewed-by: Cheyenne Wills <cwills@sinenomine.net>
Reviewed-by: Andrew Deason <adeason@sinenomine.net>
During parallel builds, we can run the final 'libtool --mode=link'
step for, say, pam_afs.la and pam_afs.krb.la in parallel. When libtool
runs --mode=link for these libraries, it effectively does the
following (amongst many other steps):
- rm -rf .libs/"$libname".*
- mkdir .libs/"$libname".lax
- mkdir .libs/"$libname".lax/libopr_pic.a
The 'rm -rf' cleans up any lingering files from previous runs in the
.libs dir (e.g. .libs/pam_afs.a, .libs/pam_afs.lai, etc etc). The
'.lax' directory is used as a temporary space to extract object files
in dependant libs. And of course it creates various other files in
there named like .libs/pam_afs.lai.
But if we're building a library called pam_afs.krb.la, then some of
our temporary files will be named like .libs/pam_afs.krb.lax, which
get deleted when building pam_afs.la when it deletes
".libs/pam_afs.*". So, if we build pam_afs.la while the libtool
command for pam_afs.krb.la is using .libs/pam_afs.krb.lax, the
temporary files can get deleted out from under us, causing rare
confusing libtool errors. For example:
/bin/sh ../../libtool --quiet --mode=link --tag=CC gcc -rpath /usr/local/lib \
-module -no-undefined -o pam_afs.la [...]
/bin/sh ../../libtool --quiet --mode=link --tag=CC gcc -rpath /usr/local/lib \
-module -no-undefined -o pam_afs.krb.la [...]
../../libtool: line 1719: cd: .libs/pam_afs.krb.lax/libopr_pic.a: No such file or directory
make[3]: *** [Makefile:76: pam_afs.krb.la] Error 1
make[3]: *** Waiting for unfinished jobs....
Fortunately, most of the libraries we build don't have dots in their
name, and so avoid this issue. But we do have a few libraries that
build a ".krb" variant, and so can be impacted by this. This includes
pam_afs.krb, libauth.krb, and libkauth.krb.
To make sure these libraries don't encounter this problem, make the
.krb variant depend on the non-.krb variant, so the libraries aren't
built in parallel.
Change-Id: Icb96a721b481bc3d99c9e24cf81fcfbbf7d498f6
Reviewed-on: https://gerrit.openafs.org/15328
Reviewed-by: Cheyenne Wills <cwills@sinenomine.net>
Reviewed-by: Stephan Wiesand <stephan.wiesand@desy.de>
Tested-by: BuildBot <buildbot@rampaginggeek.com>
Reviewed-by: Michael Meffie <mmeffie@sinenomine.net>
Since the original IBM code import, the 'bos' utility source code has
contained an unused pre-processor conditional 'OPBOS'. If OPBOS is
defined, it builds a bos binary that implements only the following bos
commands: status, start, stop, restart, startup, shutdown.
There is no configure option or any other logic that defines 'OPBOS',
nor is it documented. Remove the unused conditional.
Change-Id: I4d32084e6a6b4ca8cadb0aa0ef06c74fb6edb770
Reviewed-on: https://gerrit.openafs.org/15875
Tested-by: BuildBot <buildbot@rampaginggeek.com>
Reviewed-by: Cheyenne Wills <cwills@sinenomine.net>
Reviewed-by: Andrew Deason <adeason@sinenomine.net>
Modify OpenAFS to respect user-specified ARCHFLAGS (-m32 or -m64) when
building on Solaris 10 and 11 SPARC platforms. If ARCHFLAGS is
unset/unspecified, we will continue to build 32-bit userspace by
default.
Implement -m64 SPARC LWP via USE_UCONTEXT; leave the -m32 SPARC LWP
(default) implementation unchanged.
Kernelspace builds remain 64-bit, regardless of ARCHFLAGS value.
Change-Id: Ibb8e145d0cb8dfbe05de896b84c1d1135664b845
Reviewed-on: https://gerrit.openafs.org/14897
Reviewed-by: Andrew Deason <adeason@sinenomine.net>
Reviewed-by: Michael Meffie <mmeffie@sinenomine.net>
Tested-by: BuildBot <buildbot@rampaginggeek.com>
Currently, the ReclaimedVCList is only fed on Darwin. As a result, the
logic present in afs_FlushReclaimedVcaches() is never used on all other
platforms and should be wrapped in #ifdef AFS_DARWIN_ENV. While here,
also reorganize this function in an effort to make it more readable.
No functional change is incurred by this commit.
Change-Id: I1097a2b07fcb48dec2b31385477c34e974144b51
Reviewed-on: https://gerrit.openafs.org/15027
Reviewed-by: Cheyenne Wills <cwills@sinenomine.net>
Tested-by: BuildBot <buildbot@rampaginggeek.com>
Reviewed-by: Michael Meffie <mmeffie@sinenomine.net>
Reviewed-by: Andrew Deason <adeason@sinenomine.net>
Because the afs_vhashT hash table uses singly-linked lists in its hash
buckets, to remove an entry we have to:
1. Find the bucket where the entry to be removed should be present.
2. Go through each element of this bucket until we find this entry.
3. Remove it.
Due to step number 2, the time required to remove an entry from a bucket
increases as the linked list associated with this bucket gets longer.
This can get particularly bad when evicting vcaches (ShakeLooseVCaches),
as each entry from the least recently used queue (VLRU) has to be found
in our afs_vhashT hash table. This problem is exacerbated when files are
repeatedly deleted and recreated in the same volume, resulting in many
vcaches with the same volume id and vnode number (but different unique
ids) in the same bucket.
To avoid this problem, build afs_vhashT using doubly-linked lists with
'struct afs_q', like we did for afs_vhashTV in commit 4fc48af8
(vc-hashing-be-less-expensive-20050728). Now, removing a vcache from
afs_vhashT is an O(1) operation (QRemove).
Change-Id: I5a1a9a090f9aa3d8884e2bb12aca1f1acc3b902d
Reviewed-on: https://gerrit.openafs.org/14949
Tested-by: BuildBot <buildbot@rampaginggeek.com>
Reviewed-by: Cheyenne Wills <cwills@sinenomine.net>
Reviewed-by: Michael Meffie <mmeffie@sinenomine.net>
Reviewed-by: Marcio Brito Barbosa <mbarbosa@sinenomine.net>
Reviewed-by: Andrew Deason <adeason@sinenomine.net>
Replace strcpy, strcat, and sprintf with safer string functions in the
tests/torture/DumpAfsLog program. Add checks for string truncation
where needed.
Change-Id: I4eac12e73bbeaeadc0a8a6b5ebded664ca91b7f8
Reviewed-on: https://gerrit.openafs.org/15489
Reviewed-by: Cheyenne Wills <cwills@sinenomine.net>
Reviewed-by: Mark Vitale <mvitale@sinenomine.net>
Reviewed-by: Gaurav Saxena <gsaxena@sinenomine.net>
Reviewed-by: Michael Meffie <mmeffie@sinenomine.net>
Tested-by: Michael Meffie <mmeffie@sinenomine.net>
Remove the commented out code in the DumpAfsLog test program.
Change-Id: Ie7b380ec37d18cfc7b1be8ce9904b55ad101b263
Reviewed-on: https://gerrit.openafs.org/15871
Tested-by: BuildBot <buildbot@rampaginggeek.com>
Reviewed-by: Cheyenne Wills <cwills@sinenomine.net>
Reviewed-by: Mark Vitale <mvitale@sinenomine.net>
Reviewed-by: Michael Meffie <mmeffie@sinenomine.net>
Several static analysis tools have identified various problems:
- files left open from early returns/exits (infer)
- missing checks to ensure *alloc was successful (infer)
- memory leaks due to not freeing storage before
a return (infer)
- unused variables (cppcheck)
To resolve the above problems:
- close files before returning/exiting
- add checks to ensure *alloc was successful before using the memory
- free storage before returning
- remove unused variables
This commit is a reorganization of commits submitted by Pat Riehecky,
who ran the static analysis tools and developed the fixes.
Change-Id: If45d64c370ad68d1e844b6aa5881f9dbd75300c9
Reviewed-on: https://gerrit.openafs.org/14671
Reviewed-by: Andrew Deason <adeason@sinenomine.net>
Tested-by: BuildBot <buildbot@rampaginggeek.com>
Reviewed-by: Michael Meffie <mmeffie@sinenomine.net>
Reviewed-by: Mark Vitale <mvitale@sinenomine.net>
Reviewed-by: Marcio Brito Barbosa <mbarbosa@sinenomine.net>
There are several large #if/#endif blocks that do not have a comment on
the #endif, and some of the nested #if/#else/#endif blocks are not
indented.
In addition, there are violations of the CODING style such as
indentation errors, trailing whitespace, braces for functions that are
not on their own line, etc.
Add comments to the #endif statements for the larger blocks.
Use indent with parameters from CODING to fix the remaining style
issues, and fix up the preprocessor indentation.
There are no functional changes
Change-Id: I89a13a1f43aa08728777a71a41984613ac163b95
Reviewed-on: https://gerrit.openafs.org/15669
Reviewed-by: Andrew Deason <adeason@sinenomine.net>
Tested-by: BuildBot <buildbot@rampaginggeek.com>
Reviewed-by: Cheyenne Wills <cwills@sinenomine.net>
Reviewed-by: Michael Meffie <mmeffie@sinenomine.net>
Reviewed-by: Mark Vitale <mvitale@sinenomine.net>
Reviewed-by: Marcio Brito Barbosa <mbarbosa@sinenomine.net>
Commit de46edef69 (Remove lowercase min/max macro definitions) removed
the min() macro definition from the util/ktime.h header. Unfortunately,
that min() definition is still required for some of the Windows source
files.
Later, the windows sources can be converted to use the new in-tree
opr_min and opr_max macros, but for now reinstate the min() macro
definition in ktime.h.
Fixes the build errors on Windows:
msrpc.obj : error LNK2001: unresolved external symbol _min
RDRFunction.obj : error LNK2001: unresolved external symbol _min
cm_server.obj : error LNK2019: unresolved external symbol _min referenced
in function _cm_SetServerIPRank
cm_dcache.obj : error LNK2001: unresolved external symbol _min
cm_vnodeops.obj : error LNK2001: unresolved external symbol _min
smb3.obj : error LNK2001: unresolved external symbol _min
Change-Id: I043e23e72977ef06330b53f05044cd3be93cc00e
Reviewed-on: https://gerrit.openafs.org/15870
Reviewed-by: Michael Meffie <mmeffie@sinenomine.net>
Tested-by: Michael Meffie <mmeffie@sinenomine.net>
Tested-by: BuildBot <buildbot@rampaginggeek.com>
Commit 8e1e242ce4 (Replace lowercase min/max macros with opr_min/max)
converted the min() invocation in auth/ktc_nt.c with our in-tree
opr_min() macro, but missed adding the opr.h header to that source file.
Add the opr.h include to fix the build error on windows:
afsauth.lib(ktc_nt.obj) : error LNK2019: unresolved external symbol
_opr_min referenced in function _GetLocalToken
Change-Id: I96dbe377a8a85921fa1833cd27638cd853e26b9c
Reviewed-on: https://gerrit.openafs.org/15869
Reviewed-by: Michael Meffie <mmeffie@sinenomine.net>
Tested-by: Michael Meffie <mmeffie@sinenomine.net>
Tested-by: BuildBot <buildbot@rampaginggeek.com>
The commit:
"afsd: Move to newer cmd syntax" (fbe6f0f4ad)
removed setting the flag variable "sawBiods" when the -nbiods option is
used, this results in the -nbiods value being ignored and the default
value being used.
This problem was identified by the cppcheck static analysis tool.
Note: The -nbiods option is only for AIX systems.
Change-Id: I9a8d199a212a164660c1e9f8b4151792d9c9d0e0
Reviewed-on: https://gerrit.openafs.org/15212
Reviewed-by: Andrew Deason <adeason@sinenomine.net>
Reviewed-by: Marcio Brito Barbosa <mbarbosa@sinenomine.net>
Tested-by: BuildBot <buildbot@rampaginggeek.com>
Reviewed-by: Michael Meffie <mmeffie@sinenomine.net>
Since introduced in commit 53986479ee (ptutil-initial-20001219),
pt_util has used a statically allocated buffer to read pages from the
database. In several places the buffer is cast to a struct ubik_header.
Memory allocated dynamically via malloc()/calloc() is guaranteed to be
properly aligned for objects of any type at run time, however buffers
that are not allocated dynamically have no such guarantee. So, instead
of using a statically allocated buffer, dynamically allocate it with
calloc() to ensure it is aligned for struct ubik_header objects.
This fixes a SIGBUS error whenever the buffer happens to be built with a
non-word alignment. This failure was observed while running the
tests/ptserver/pt_util-t unit test on Solaris SPARC 64-bit.
Note: By inspection, the same potential problem exists in
src/kauth/ka_util.c. However, kaserver is deprecated and ka_util has
never been built by default.
Change-Id: I00372ed6adede32448b5e0df1c1d74689286a218
Reviewed-on: https://gerrit.openafs.org/15352
Tested-by: BuildBot <buildbot@rampaginggeek.com>
Reviewed-by: Cheyenne Wills <cwills@sinenomine.net>
Reviewed-by: Mark Vitale <mvitale@sinenomine.net>
Reviewed-by: Michael Meffie <mmeffie@sinenomine.net>
rxkad/fcrypt defines 2 macros: ENCRYPT and DECRYPT. The names are
pretty generic and do not clearly associate with fcrypt.
In addition, the name ENCRYPT collide with the Microsoft provided header
'ntddndis.h' that uses ENCRYPT as an ENUM (on newer versions of the
Windows development kits):
typedef enum _OFFLOAD_OPERATION_E
{
AUTHENTICATE = 1,
ENCRYPT
}
This results in a build error on Windows when fcrypt.h is included with
ntddndis.h:
... ntddndis.h(2212): error C2059: syntax error: 'constant'
Rename the ENCRYPT/DECRYPT macros to FCRYPT_ENCRYPT/FCRYPT_DECRYPT in
fcrypt.h to a name that relates the macros back to fcrypt.
Note: The ENCRYPT/DECRYPT symbols are part of a public interface
installed in fcrypt.h, but keeping the old names are impractical, so we
are changing them anyway.
Change-Id: I9c51c81fab7fcbf0dae01569852ca94c0e6a0439
Reviewed-on: https://gerrit.openafs.org/15868
Tested-by: BuildBot <buildbot@rampaginggeek.com>
Reviewed-by: Michael Meffie <mmeffie@sinenomine.net>
Reviewed-by: Benjamin Kaduk <kaduk@mit.edu>
Several static analysis tools have identified various problems:
- missing checks to ensure *alloc was successful (infer)
To resolve the above problems:
- add checks to ensure *alloc was successful before using the memory
This commit is a reorganization of commits developed by Pat Riehecky,
who ran the static analysis tools and developed the fixes.
Change-Id: I9396b8327f8283c60821050c75de1c36e0ebd12e
Reviewed-on: https://gerrit.openafs.org/14684
Tested-by: BuildBot <buildbot@rampaginggeek.com>
Reviewed-by: Cheyenne Wills <cwills@sinenomine.net>
Reviewed-by: Michael Meffie <mmeffie@sinenomine.net>
CheckVolume accepts an optional nvldbentry as an argument. When this
argument is NULL, CheckVolume correctly locks the nvldbentry before
obtaining the entry and making changes. However, if an nvldbentry is
passed, CheckVolume merely copies it to a work entry; it also neglects
to obtain the current state of the entry after locking it for
modification.
Change CheckVolume to obtain a locked copy of the nvldbentry if
modifications are required.
Change-Id: Ie98bc57af1cc2955d10982f44abd4c55755c6244
Reviewed-on: https://gerrit.openafs.org/14356
Tested-by: BuildBot <buildbot@rampaginggeek.com>
Reviewed-by: Cheyenne Wills <cwills@sinenomine.net>
Reviewed-by: Michael Meffie <mmeffie@sinenomine.net>
Reviewed-by: Mark Vitale <mvitale@sinenomine.net>
Reviewed-by: Andrew Deason <adeason@sinenomine.net>
In preparation for a future commit, refactor some large chunks of logic
out of CheckVolume and into new static routines to handle the specific
cases for RW, RO, and BK volumes. Also provide Doxygen documentation for
CheckVolume.
No functional change should be incurred by this commit.
Change-Id: Iac6a773d29ed530e9676f02726db031b1f2a2526
Reviewed-on: https://gerrit.openafs.org/14355
Tested-by: BuildBot <buildbot@rampaginggeek.com>
Reviewed-by: Cheyenne Wills <cwills@sinenomine.net>
Reviewed-by: Michael Meffie <mmeffie@sinenomine.net>
Reviewed-by: Mark Vitale <mvitale@sinenomine.net>
Reviewed-by: Andrew Deason <adeason@sinenomine.net>
Since the original IBM code import, UV_RestoreVolume (later renamed to
to the current name UV_RestoreVolume2) has called VLDB_GetEntryByID to
obtain the VLDB entry of the target/destination volume. This entry is
then modified to match the restore changes, locked via VL_SetLock, and
replaced via VL_ReplaceEntryN.
Unfortunately, this allows for a race with other volume operations
against the same volume, if another operation alters the entry after we
call VLDB_GetEntryByID but before ubik_VL_SetLock. To avoid this, call
GetLockedEntry to ensure that the entry is locked while we are reading
and modifying it.
Change-Id: I772ef80158205f45c04f8ef0aa95726f1f2c4ac4
Reviewed-on: https://gerrit.openafs.org/14354
Tested-by: BuildBot <buildbot@rampaginggeek.com>
Reviewed-by: Michael Meffie <mmeffie@sinenomine.net>
Reviewed-by: Cheyenne Wills <cwills@sinenomine.net>
Reviewed-by: Mark Vitale <mvitale@sinenomine.net>
Reviewed-by: Andrew Deason <adeason@sinenomine.net>
Since the original IBM code import, UV_RestoreVolume2 (originally
UV_RestoreVolume) has checked for VL_ENTDELETED from VLDB_GetEntryByID.
However, if VL_ENTDELETED is received, we will act like we retrieved the
vlentry successfully. If the -verbose flag is given, we'll try to print
the entry (even though one was not returned), and we'll call VL_SetLock
for the volume, which will almost certainly also fail with VL_ENTDELETED
and bail out.
Furthermore (also since the original IBM import), there is no longer any
code path to set the volume entry flag VLDELETED. So it should be rare
for any VL_* RPC to return VL_ENTDELETED; that would require an entry to
have the VLDELETED flag carried over from a very old database.
Therefore, it is safe and reasonable to simply remove the check for
VL_ENTDELETED from UV_RestoreVolume2. If we do somehow receive the
VL_ENTDELETED error code, vos will terminate with an error instead of
possibly printing an uninitialized entry.
In addition, remove the same check for VL_ENTDELETED in the equivalent
code path in libadmin's UV_RestoreVolume.
Change-Id: I11d1c3306f67d68de54780f6aac75e4c27779db4
Reviewed-on: https://gerrit.openafs.org/14357
Reviewed-by: Michael Meffie <mmeffie@sinenomine.net>
Reviewed-by: Andrew Deason <adeason@sinenomine.net>
Tested-by: Andrew Deason <adeason@sinenomine.net>
In several routines in vsprocs.c, we declare and clear nvldbentry
storeEntry long before it is actually needed.
Instead, where possible, move the declaration and memset of storeEntry
closer to the actual point of use.
No functional change is incurred by this commit.
Change-Id: Iffbf5ff56fab4d221586dfb93bbbbfde063c856e
Reviewed-on: https://gerrit.openafs.org/14353
Reviewed-by: Cheyenne Wills <cwills@sinenomine.net>
Reviewed-by: Marcio Brito Barbosa <mbarbosa@sinenomine.net>
Reviewed-by: Andrew Deason <adeason@sinenomine.net>
Tested-by: BuildBot <buildbot@rampaginggeek.com>
Reviewed-by: Michael Meffie <mmeffie@sinenomine.net>
Reviewed-by: Mark Vitale <mvitale@sinenomine.net>
Change UV_ReleaseVolume() to use the new GetLockedEntry() function. Add
logic to GetLockedEntry() to support the special semantics of VL_SetLock
for VLOP_RELEASE, to preserve the current behavior of UV_ReleaseVolume()
with the VL_RERELEASE error code.
The special semantics of the VL_RERELEASE error code with VLOP_RELEASE
are a bit odd: this situation can currently only happen with our
vlserver if the vlentry has the VLOP_RELEASE flag set but no lock
timestamp. The only way that situation can occur with our current 'vos'
is during a small window after a partially failed 'vos release'. Around
where UV_ReleaseVolume() prints the message "The volume %lu could not be
released to the following %d sites", we call VLDB_ReplaceEntry() with
LOCKREL_TIMESTAMP, clearing just the lock timestamp but not the lock
flags. We then jump to the 'rfail' label where we ubik_VL_ReleaseLock()
with all LOCKREL_* flags are set, clearing the lock flags.
So, the only time in this codepath where the lock timestamp is cleared
but the VLOP_RELEASE flag is set is after the mentioned
VLDB_ReplaceEntry() call but before the ubik_VL_ReleaseLock() call. If
that last ubik_VL_ReleaseLock() fails, or if vos dies before reaching
it, then the vlentry is left in a state that causes the VL_RERELEASE
error code in the future. This behavior has existed since at least
OpenAFS 1.0.
Currently, the handling of this special VL_RERELEASE case is completely
silent, so it's not clear that this is happening, if it does manage to
happen. These special semantics are not really safe, since two vos
processes could encounter a VL_RERELEASE volume at the same time, and go
through the 'vos release' process in parallel thinking they have the
vlentry locked, causing all kinds of problems.
This behavior may be changed in the future, but for now just preserve
the existing special handling of VL_RERELEASE. Add a warning message,
though, to make this special situation not silent.
No functional change (other than error messages) should be incurred by
this commit.
[adeason@sinenomine.net: Add VL_RERELEASE warning message and additional
related comments.]
Change-Id: I9da50677233f63235de730b5d234a9b575e196fd
Reviewed-on: https://gerrit.openafs.org/14352
Tested-by: BuildBot <buildbot@rampaginggeek.com>
Reviewed-by: Michael Meffie <mmeffie@sinenomine.net>
Reviewed-by: Cheyenne Wills <cwills@sinenomine.net>
Reviewed-by: Mark Vitale <mvitale@sinenomine.net>
Reviewed-by: Andrew Deason <adeason@sinenomine.net>
In preparation for a future commit, consolidate common VL_SetLock and
VL_GetEntryById logic into a new routine GetLockedEntry.
This commit should incur no practical change in behavior, but there is a
minor internal behavior change for one case:
For the case in UV_ConvertRO(), previously we called VL_SetLock with the
RW volume id (with volume type RWVOL), and called VL_GetEntryByID with
the RO volume id (with the volume type -1); now we call both with the RW
volume id. We get these volume IDs from the same VL entry, and they must
refer to the same VL entry in order for this function to work properly,
so this difference should not matter.
Change-Id: I76faf3d184c397bceff27ba3a857b5c80c88e814
Reviewed-on: https://gerrit.openafs.org/14350
Tested-by: BuildBot <buildbot@rampaginggeek.com>
Reviewed-by: Michael Meffie <mmeffie@sinenomine.net>
Reviewed-by: Cheyenne Wills <cwills@sinenomine.net>
Reviewed-by: Mark Vitale <mvitale@sinenomine.net>
Reviewed-by: Andrew Deason <adeason@sinenomine.net>
rxi_DestroyConnectionNoLock contains the following early return
test:
if ((conn->refCount > 0) || (conn->flags & RX_CONN_BUSY)) {
/* Busy; wait till the last guy before proceeding */
MUTEX_EXIT(&rx_refcnt_mutex);
MUTEX_EXIT(&conn->conn_data_lock);
USERPRI;
return;
}
This prevents the rx_connection ("conn") from being destroyed if it is
still in use elsewhere, e.g.:
- the rx_Listener is handling packets for one of the conn's rx_calls
- another caller has a reference to the conn
However, since the original IBM code import, rxi_ReceivePacket obtains a
conn reference (via rxi_FindConnection) but returns it by merely
decrementing the refCount (currently via putConnection), and does not
call rx_DestroyConnection. If the listener's rxi_ReceivePacket is
processing a reply or ack for a client conn (holding a reference) while
another thread is calling rx_DestroyConnection on the otherwise-last
reference, the rx_connection will not be destroyed and can never be
destroyed - it has been leaked.
Modify rxi_ReapConnections to destroy client conns with refCount == 0.
This way, these connections can still "leak", but they're eventually
cleaned up the next time rxi_ReapConnections runs.
Change-Id: I8c588d43b108a8147a07d0ff0cc69055cd00d355
Reviewed-on: https://gerrit.openafs.org/15135
Reviewed-by: Cheyenne Wills <cwills@sinenomine.net>
Reviewed-by: Michael Meffie <mmeffie@sinenomine.net>
Tested-by: BuildBot <buildbot@rampaginggeek.com>
Reviewed-by: Andrew Deason <adeason@sinenomine.net>
rxi_ReapConnections obtains conn_data_lock and rx_refcnt_mutex for each
server connection in the rx_connHashTable.
Instead, only obtain these locks for server connections that have no
calls.
Change-Id: I27d748e604070792bee3c1f31d4d558462fd399a
Reviewed-on: https://gerrit.openafs.org/15349
Reviewed-by: Cheyenne Wills <cwills@sinenomine.net>
Tested-by: BuildBot <buildbot@rampaginggeek.com>
Reviewed-by: Michael Meffie <mmeffie@sinenomine.net>
Reviewed-by: Andrew Deason <adeason@sinenomine.net>
Both the Unix and Windows cache managers maintain a set of persistent
client rx_connections ("conns") to the known fileservers for each active
AFS user. These conns are periodically refreshed (destroyed and
possibly rebuilt) approximately NOTOKETIMEOUT (2h) after token
expiration for authenticated (rx_kad) conns, or every NOTOKETIMEOUT (2h)
for anonymous (rx_null) conns.
Both cache managers enable NAT ping for one rx_connection to each known
fileserver. Thus we see this common idiom when a cache manager destroys
a fileserver connection:
rx_SetConnSecondsUntilNatPing(conn, 0);
rx_DestroyConnection(conn);
Doing this for all conns is harmless, even if a given conn doesn't have
NAT ping enabled.
It is important to note that rx_SetConnSecondsUntilNatPing(conn, 0) does
not actually cancel the conn's natKeepAliveEvent (if it has one); it
merely sets conn->secondsUntilNatPing to 0. If there is a
natKeepAliveEvent, this prevents it from being rescheduled after the
next event. This was fine in the past because rx_DestroyConnection
eventually did cancel natKeepAliveEvent and destroy the rx_connection
itself.
However, this idiom broke after commit 304d758983 "Standardize
rx_event usage" introduced a number of changes:
- an extra rx_connection refCount for each outstanding conn event
- a requirement (enforced by osi_Assert) that all conn events be
canceled before rx_DestroyConnection can succeed
Therefore, if natKeepAliveEvent is still active when we enter
rx_DestroyConnection, it will return early, due to the extra conn
refCount associated with the NAT ping event. The rx_connection is not
destroyed in this case.
Eventually, the final scheduled natKeepAliveEvent will fire; the event
will not be rescheduled, and the final conn refCount will be removed via
putConnection and not rx_DestroyConnection (and so the conn is not
destroyed). This rx_connection now has refCount 0, but can never be
destroyed - it has been leaked:
- The cache manager has "forgotten" this rx_connection and has no means
to invoke rx_DestroyConnection again.
- rxi_ReapConnections will not destroy it because it is a client conn
and is still on the rx_connHashTable, not the rx_connCleanup_list.
- If there is still a dallying rx_call, any eventual call to
rxi_CheckCall -> rxi_FreeCall will not destroy the conn because it
has not been flagged RX_CONN_DESTROY_ME.
Modify rx_SetConnSecondsUntilNatPing to explicitly cancel any
natKeepAliveEvent and remove its refcount if cancelled. With this
change in place, the cache managers will no longer periodically leak
client rx_connections.
Change-Id: I4e89ebc4bd2c95b6e61b95bd8f91867d451dd34c
Reviewed-on: https://gerrit.openafs.org/14951
Reviewed-by: Cheyenne Wills <cwills@sinenomine.net>
Tested-by: BuildBot <buildbot@rampaginggeek.com>
Reviewed-by: Michael Meffie <mmeffie@sinenomine.net>
Reviewed-by: Andrew Deason <adeason@sinenomine.net>
The cmd_Parse() function calls strdup() to create a temporary string
when there is a parameter in the form "-<name>=<value>". This is
done in order to parse the <name> and <value> components of the
parameter.
This memory is not freed when the last parameter of the command is the
form "-<name>=<value>". Always free the param pointer at the end of
cmd_Parse() to avoid this memory leak.
This memory leak was found by running the tests/cmd/command-t unit test
program with valgrind.
Change-Id: I85f8f5f10660268596072c8bb8be2c014ac46ad8
Reviewed-on: https://gerrit.openafs.org/15086
Reviewed-by: Mark Vitale <mvitale@sinenomine.net>
Tested-by: BuildBot <buildbot@rampaginggeek.com>
Reviewed-by: Cheyenne Wills <cwills@sinenomine.net>
Reviewed-by: Michael Meffie <mmeffie@sinenomine.net>
A prior commit:
"Replace lowercase min/max macros with opr_min/max"
(Change-Id: Ib7c91b4e59aaf76ab77f293016496d6179c23c88)
replaced the uses of the min and max macros with opr_min and opr_max.
As a cleanup, remove the definitions for the min() and max() macros.
As noted in the above commit, there were several locations that left
the min/max macros alone:
src/WINNT
src/external/heimdal
Change-Id: I263d91b04a2aed14bf9e1ebca1616629f36e6b23
Reviewed-on: https://gerrit.openafs.org/15843
Reviewed-by: Cheyenne Wills <cwills@sinenomine.net>
Tested-by: BuildBot <buildbot@rampaginggeek.com>
Reviewed-by: Andrew Deason <adeason@sinenomine.net>
Reviewed-by: Michael Meffie <mmeffie@sinenomine.net>
The commit:
"opr: replace MIN/MAX macros with opr_min/opr_max" (915c5cff16)
replaced all uses of the MIN and MAX macros with opr_min and opr_max.
Since we now have a consolidated version of opr_min and opr_max,
replace the min/max macros with opr_min and opr_max, except for the
files in src/WINNT (which can be handled in a future commit).
The exception are the min/max macros that are defined and used in the
external sources src/external/heimdal, which cannot be changed.
Change-Id: Ib7c91b4e59aaf76ab77f293016496d6179c23c88
Reviewed-on: https://gerrit.openafs.org/15825
Tested-by: BuildBot <buildbot@rampaginggeek.com>
Reviewed-by: Andrew Deason <adeason@sinenomine.net>
Reviewed-by: Michael Meffie <mmeffie@sinenomine.net>
When restoring with a regular file (either with -file or a shell redirect
'<'), vos restore verifies the file ends with the end-of-dump magic
number. This catches cases in which the specified file is obviously not
a dump file, for example the file is empty, truncated, or just not file
created by vos dump.
Currently, no checks are done when restoring from a pipe or fifo, since it
is not possible to check the end of the stream and reset the seek pointer
to before sending the data to the volume server.
Add a check to verify the dump begins with the dump header tag and the
start-of-dump magic number, even when the dump file is not seekable.
In the case the dump file is not seekable, send the start of dump
tag and the start-of-dump magic number directly in SendFile(), since
those values were already read in CheckDumpFile().
This guards against starting a restore from an empty or wrong file when
restoring from a pipeline or a named pipe.
Change-Id: I7aa508bf5f0e81da7d6602856b5242cb6313e9a8
Reviewed-on: https://gerrit.openafs.org/14711
Reviewed-by: Cheyenne Wills <cwills@sinenomine.net>
Tested-by: BuildBot <buildbot@rampaginggeek.com>
Reviewed-by: Andrew Deason <adeason@sinenomine.net>
When restoring a volume with a regular file, vos restore verifies the dump
file is terminated with the end-of-dump magic number before sending the
dump to the volume server. Unfortunately, this check is done after the
target volume is deleted (which happens when doing a full restore of a
volume), in which case the target volume is unnecessarily removed.
Currently, UV_RestoreVolume2() invokes the WriteData() function as
callback to transmit the dump stream to the volume server. WriteData()
opens the usd file handle, performs the end-of-dump magic check (if the
file is seekable), sends the data using the SendFile() helper function,
then closes the dump file handle.
Instead, open and check the dump file magic before calling
UV_RestoreVolume2(). Pass the usd file handle and a pointer to the
SendFile() function to UV_RestoreVolume2() to send the dump data to the
volume server.
Move the code to perform the dump file magic check to the new
CheckDumpFile() helper function and remove the now unneeded WriteData()
function.
Change-Id: Ia04f378540b40c2e6360aa120592fbd793c33cae
Reviewed-on: https://gerrit.openafs.org/14710
Reviewed-by: Cheyenne Wills <cwills@sinenomine.net>
Tested-by: BuildBot <buildbot@rampaginggeek.com>
Reviewed-by: Andrew Deason <adeason@sinenomine.net>
When doing a full volume restore (vos restore -overwrite full), the
target volume is deleted by UV_RestoreVolume2() before the dump file is
opened.
After the target volume is deleted, UV_RestoreVolume2() calls the
WriteData() callback function to open the dump file and send the volume
data to the volume server. If the volume dump file cannot be opened at
that point, then the volume restore operation is aborted before sending
any dump data, but the target volume was unnecessarily destroyed.
Currently, a precheck is done by the FileExists() function, but the file
handled acquired by FileExists() is discarded, and a new file handle is
opened in the WriteData() function.
To improve this situation, change the sequence to open the dump file
before calling UV_RestoreVolume2() and pass the opened file handle,
instead of the filename string, and close the file handle after
UV_RestoreVolume2() returns.
The FileExists() function is no longer needed since we no longer discard
the opened file handle when processing the vos restore arguments, so
remove the now unused FileExists() function.
Also remove the afilename string from RestoreVolumeCmd() and pass the
opened file handle to UV_RestoreVolume2() instead.
Because of this refactoring, we no longer report an error when the
USC_CLOSE of the dump file handle fails, and we no longer run
USD_IOCTL_GETSIZE when opening the dump file (and exit with an error if
it fails.)
Change-Id: I6092e43a5dcaa795191143b65388ad1117b582f3
Reviewed-on: https://gerrit.openafs.org/15069
Reviewed-by: Cheyenne Wills <cwills@sinenomine.net>
Tested-by: BuildBot <buildbot@rampaginggeek.com>
Reviewed-by: Andrew Deason <adeason@sinenomine.net>
Add a check to avoid dumping a volume to the user's terminal when an
output file is not specified and the user forgets to pipe or redirect
stdout.
Change-Id: Ifdef6550d37981497ac2e8cb596993c2ed2e58f2
Reviewed-on: https://gerrit.openafs.org/14778
Reviewed-by: Michael Meffie <mmeffie@sinenomine.net>
Reviewed-by: Cheyenne Wills <cwills@sinenomine.net>
Tested-by: BuildBot <buildbot@rampaginggeek.com>
Reviewed-by: Andrew Deason <adeason@sinenomine.net>
Currently, vos restore assumes the dump stream is provided via stdin
when the -file option is not specified and does not check to see if
stdin is a tty.
When restoring over an existing volume and the -overwrite is not
specified, vos will try to prompt the user for the overwrite mode
(incremental, full, or abort). Currently, vos will not prompt the user
if the -file option is also absent, since the assumption is the dump is
to be read from stdin.
Instead, read the dump from stdin only when the -file option is absent
and stdin is not a tty, and only prompt the user when stdin is a tty.
This prevents vos restore from hanging when a dump file was not given
with the -file option, a pipeline, or a redirect.
This change also removes the use of the dump "afilename" variable to
determine if an interactive mode should be used.
Change-Id: I33edb69c9c3c6f42fc2270c1c797be862ffa2773
Reviewed-on: https://gerrit.openafs.org/14760
Reviewed-by: Cheyenne Wills <cwills@sinenomine.net>
Reviewed-by: Marcio Brito Barbosa <mbarbosa@sinenomine.net>
Tested-by: BuildBot <buildbot@rampaginggeek.com>
Reviewed-by: Andrew Deason <adeason@sinenomine.net>
When restoring a volume with the -file argument, vos verifies the last
four bytes of the file are the dump end magic before sending the dump to
the volume server. Currently, this check is skipped when reading a dump
from stdin because it is assumed the data is being read from a pipe.
However, this assumption is not correct. The file handle created from
stdin may be seekable when redirection is used, and the file handle
created from a file name may or may not be seekable, depending on the
file type.
For example, when redirecting a regular file to stdin, the file handle
is seekable, since the shell opens the given file name and associates
stdin with the opened file:
$ vos restore ... < myfile
In addition, the file handle is not seekable when specifying a named
pipe (fifo) with the -file option:
$ mkfifo mypipe
$ vos restore ... -file mypipe
Instead of assuming file handles opened from file names are always
seekable and file handles opened from stdin are never seekable, use
USD_IOCTL() to determine when file handles are seekable, and when
seekable, verify the file ends with the dump end magic number.
While here, be sure to check the return codes from seek and reads while
checking for the dump end magic.
Change-Id: I9d16b13682365b82cb9d0b3673c4ed7c3ab4dc2e
Reviewed-on: https://gerrit.openafs.org/14758
Reviewed-by: Andrew Deason <adeason@sinenomine.net>
Reviewed-by: Cheyenne Wills <cwills@sinenomine.net>
Tested-by: BuildBot <buildbot@rampaginggeek.com>
Reviewed-by: Michael Meffie <mmeffie@sinenomine.net>
Add the USD_IOCTL_ISSEEKABLE verb to USD_IOCTL() to check if USD_SEEK()
can be used on usd file handles.
Change-Id: I72f4509f39c82ec0ca4c0983b4c804e490b97ea0
Reviewed-on: https://gerrit.openafs.org/14777
Reviewed-by: Cheyenne Wills <cwills@sinenomine.net>
Reviewed-by: Marcio Brito Barbosa <mbarbosa@sinenomine.net>
Reviewed-by: Andrew Deason <adeason@sinenomine.net>
Tested-by: BuildBot <buildbot@rampaginggeek.com>
Reviewed-by: Michael Meffie <mmeffie@sinenomine.net>
Use the USD_IOCTL() function to get the block size when writing to
stdout or reading from stdin, as well as regular files.
This makes the code more consistent and eliminates the need to pass the
block size to the ReceiveFile() and SendFile() functions.
Change-Id: If45e93530ded3edc8673370cd88f18228bc21827
Reviewed-on: https://gerrit.openafs.org/14757
Reviewed-by: Cheyenne Wills <cwills@sinenomine.net>
Reviewed-by: Marcio Brito Barbosa <mbarbosa@sinenomine.net>
Reviewed-by: Andrew Deason <adeason@sinenomine.net>
Tested-by: BuildBot <buildbot@rampaginggeek.com>
Reviewed-by: Michael Meffie <mmeffie@sinenomine.net>
The file handle close error messages in the vos dump and restore
commands have "STDIN" and "STDOUT" swapped. Just remove the filename or
"STDIN"/"STDOUT" from those messages.
Remove the extra flag to indicate if we have a file handle and just
initialize the usd file handles to NULL.
This is a clean up in preparation for other changes.
Change-Id: I97bf5a8177269331e4060746736a882892b9062e
Reviewed-on: https://gerrit.openafs.org/14756
Reviewed-by: Cheyenne Wills <cwills@sinenomine.net>
Reviewed-by: Marcio Brito Barbosa <mbarbosa@sinenomine.net>
Reviewed-by: Andrew Deason <adeason@sinenomine.net>
Tested-by: BuildBot <buildbot@rampaginggeek.com>
Analysis of the source for code for copyauth has flagged potential
string buffer overflows due to the use of strcpy. Attempting to fix the
use of strcpy, would require additional work since copyauth currently
fails to build due to unresolved external references during linking.
The copyauth command has not been built by default on non-Windows
systems since 2009, 'curpag-via-pioctl-20090603' (4af75fe96a), and never
has been built by default on Windows system since the initial git
commit for openafs.
According to the man page, the functionality of copyauth has been
superseded by aklog, there is also a caution noted about using copyauth
due to security concerns.
Remove the copyauth utility and the associated references.
Change-Id: I96ba9af341bc97a329132ed4fd39f3b567d0ea4a
Reviewed-on: https://gerrit.openafs.org/15480
Reviewed-by: Andrew Deason <adeason@sinenomine.net>
Tested-by: BuildBot <buildbot@rampaginggeek.com>
Reviewed-by: Michael Meffie <mmeffie@sinenomine.net>
We generally tend to explicitly NULL out the buffer argument if
asprintf() returns failure, since it is not guaranteed to have done
so itself. It is even less clear whether setting the buffer argument
to NULL ourselves before calling asprintf() will be untouched in all
possible failures, so re-zero it after failed calls just to be sure
we don't try to free garbage later.
Change-Id: Ie8995472aeadb8043f5e43f4e6d197a7be1363e0
Reviewed-on: https://gerrit.openafs.org/15832
Tested-by: BuildBot <buildbot@rampaginggeek.com>
Reviewed-by: Michael Meffie <mmeffie@sinenomine.net>
It's been that way since the original IBM import.
Change-Id: Ib29ec60822a1ba03e53f3a942e939cc68011a5cf
Reviewed-on: https://gerrit.openafs.org/15834
Tested-by: BuildBot <buildbot@rampaginggeek.com>
Reviewed-by: Michael Meffie <mmeffie@sinenomine.net>
This comment is a very simplified summary of what bosserver does
in general, and is nowhere close to the code actually doing it,
so just remove it.
Change-Id: I09473a507787a57c8e760cd5a5d7ae11bf8dbba6
Reviewed-on: https://gerrit.openafs.org/15833
Tested-by: BuildBot <buildbot@rampaginggeek.com>
Reviewed-by: Michael Meffie <mmeffie@sinenomine.net>
We use spaces around binary '-' but not around unary '-', so tighten
up the casted expression "(pid_t)-1" and add some parens around (-1)
as well since we're here, for improved readability.
Change-Id: Ic9ad30a72ece2e0d4e53a1623e24433970fa5441
Reviewed-on: https://gerrit.openafs.org/15790
Tested-by: BuildBot <buildbot@rampaginggeek.com>
Reviewed-by: Michael Meffie <mmeffie@sinenomine.net>
Commit 466e8cb15e ('bozo: retry start
after error stops') introduced several problems for 'simple' bnodes that
experience startup errors. After its error retries have been exhausted,
the bnode continues to appear as if it is starting up. For instance,
'bos stop' is required before 'bos delete' will work. Also, if 'bos
stop -wait' is issued for a different bnode, the command will hang due
to BOZO_WaitAll waiting indefinitely for the bnode that has exhausted
its error retries.
Instead, introduce bnode_IsErrorRetrying and modify ez_getstat to call
it. In this way ex_getstat will only return BSTAT_STARTINGUP if the
error retries have not been exhausted yet. While here, also modify
ez_procexit and SetNeedsClock to consolidate all equivalent logic in
bnode_IsErrorRetrying.
Change-Id: I29d419d76a889e13049116fa66d1a63d11c16b46
Reviewed-on: https://gerrit.openafs.org/13376
Tested-by: BuildBot <buildbot@rampaginggeek.com>
Reviewed-by: Michael Meffie <mmeffie@sinenomine.net>
Reviewed-by: Cheyenne Wills <cwills@sinenomine.net>
Several files in src/tests fail to build due to compiler warnings when
building with --enable-checking. There are also unresolved references
during the link steps.
The warnings are for
implicit-fallthrough
unused-result
unused-but-set-variable
invalid-source-encoding
In addition the "all" make target references a variable that is set
later in the file, so the resulting list of targets is incorrect and are
not built.
Move libopr.a, ${LIB_hcrypto} and ${LIB_roken} from INT_LIBS into
COMMON_LIBS to resolve link failures in:
test-setgroups
test-setpag
dup2-and-unlog
create-stat
rm-rf
write-closed2
afscp
Update code that prints usage to use warnx instead of relying on
__progname.
Check for errors for various syscalls, to avoid "unused-result"
warnings.
Remove 'read_buf1' in read-write.c to avoid an "unused-but-set-variable"
warning.
Add a missing 'break' to a switch statement in utime-file.c to avoid an
"implicit-fallthrough" warning. Add an AFS_FALLTHROUGH to fsx.c to avoid
the same warning; it seems like this may be a mistake and maybe should
be a "break", but match the existing fsx.c behavior at least for now,
since the intended behavior isn't completely clear.
Relocate the Makefile "all" target so it follows the macro definition
that it references.
Rename the top level Makefile target from tests to srctests for the
recipe that invokes make to build src/tests. Add the necessary
dependencies to the srctests target. Add srctests to the
finale_notest/finale_nolibafs_notest targets
Make sure the 'OpenAFS' dir exists when generating OpenAFS/Dirpath.pm
and OpenAFS/Dirpath.sh. For objdir builds, it may not exist when we
try to generate these files.
Note The code within the src/tests directory may be out of date. This
commit only ensures that the src/tests can build cleanly. Further work
will be needed to validate the tests and ensure that they are
still working as intended.
Change-Id: Ic6d685c0dde5df25da175fe4516ce28a26443533
Reviewed-on: https://gerrit.openafs.org/15342
Tested-by: BuildBot <buildbot@rampaginggeek.com>
Reviewed-by: Michael Meffie <mmeffie@sinenomine.net>
Reviewed-by: Cheyenne Wills <cwills@sinenomine.net>
It seems that some early authors had a conversation with each other
in the comments (including some typos), but since the RPC semantics
are now locked in, we should just describe what's happening and how
it could change in the future (which seems unlikely, in any case).
Change-Id: Ic959e662609427ac873e554369cb4d2bdb6a2558
Reviewed-on: https://gerrit.openafs.org/15791
Tested-by: BuildBot <buildbot@rampaginggeek.com>
Reviewed-by: Michael Meffie <mmeffie@sinenomine.net>
Object directory builds that are configured to include CTF debuginfo
(e.g., Solaris with ctf tools installed and configure --enable-debug)
will fail with:
make
...
sh: line 1: /export/home/mvitale/src/sna-openafs-master/src/config/cc-wrapper: not found
...
Modify the path for CC_WRAPPER and LD_WRAPPER to point to the object
directory, not the source directory.
Change-Id: I972be95e7cd7b5dbac3ca6d8d76077cb152618c2
Reviewed-on: https://gerrit.openafs.org/15815
Tested-by: BuildBot <buildbot@rampaginggeek.com>
Tested-by: Michael Meffie <mmeffie@sinenomine.net>
Reviewed-by: Michael Meffie <mmeffie@sinenomine.net>
Since the original IBM code import, SalvageCmd in bos.c has had some
inaccurate comments. Correct them.
No functional change is incurred by this commit.
Change-Id: I325fecfebe17a384decb3ad345ec83a3b0ed51e7
Reviewed-on: https://gerrit.openafs.org/15838
Reviewed-by: Cheyenne Wills <cwills@sinenomine.net>
Tested-by: BuildBot <buildbot@rampaginggeek.com>
Reviewed-by: Michael Meffie <mmeffie@sinenomine.net>
Currently, if someone calls ViceLog() or similar before the logging
subsystem is setup, the messages are lost. No code currently does this
(and if it did, it would be a bug), but the potential for lost log
messages is not ideal.
To avoid this, change serverLog.c to log such messages to stderr if
logging hasn't been setup, so messages are not lost, and to make it
easier for command-line utilities to show information logged via
ViceLog/FSLog.
With this commit, now any code in a library or subsystem that logs
something via ViceLog(0) will actually show up in command-line
utilities. No such subsystem that's actually used by any command-line
utilities makes use of ViceLog, but this may change in future commits.
Change-Id: I0df4148d8bd450f5efd1ec7d8dd85f5aaececd1a
Reviewed-on: https://gerrit.openafs.org/14025
Reviewed-by: Cheyenne Wills <cwills@sinenomine.net>
Reviewed-by: Kailas Zadbuke <kailashsz@in.ibm.com>
Tested-by: BuildBot <buildbot@rampaginggeek.com>
Reviewed-by: Michael Meffie <mmeffie@sinenomine.net>
