#55 bpcancel silent failure when it should succeed

[Wes Eddy]

On Linux with ION 3.1.3:

Summary: there are 2 sub-issues here; one maybe hard, the other is definitely easy.
1) bpcancel silently returns without indicating success or failure, but isn't actually working (this is the hard one)
2) the bpcancel.c error checking on the findBundle return value has bad logic, and should probably check for "!= 1" or "<= 0" rather than the current "< 0"

More detail:

I've occasionally had an SDR state where:

• bplist shows several bundles, which look like valid ones generated by my tests
• bpcancel on them runs without indicating any errors or success
• bplist still shows the bundles

By tracing the calls a little bit, I was able to determine that:

• the bpcancel.c call to findBundle is returning 1 (which is the bset.count from sdr_read of a BundleSet from the sdr_hash_retrieve), however the bundleAddr value in findBundle is being set to 0.
• I didn't trace into the SDR functions to figure out how/why bset.count could be 1, but the actual pointer could be 0. I know it should be 0 for values > 1, per a comment in bpP.h. It's possible that I'm somehow winding up with a pathological SDR configuration.

But, in any case, the check on the findBundle return value in bpcancel.c is certainly wrong, since it uses "< 0", but findBundle actually returns 0 if it can't find a bundle, which means it should possibly be a "<= 0" instead. However, due to the way the pointer gets used later on, I think it should actually require the return value to be "== 1", so that it only tries to destroy the bundle if exactly 1 bundle matching the criteria is found.

[Scott Burleigh]

Sub-issue 1 is actually the easy one: it's simple to modify bpcancel.c to print a message indicating whether or not it was able to cancel transmission of the indicated bundle.

Sub-issue 2 requires some additional explanation. First, the man page for bpcancel needs to be revised to say that bpcancel attempts to cancel transmission of a specified bundle, not that it destroys a specified bundle. These are the same in the event that there's never more than one copy of the bundle in the node's bundle store, but not when there are multiple copies; this can happen when you use IMC multicast or when you set the Critical flag in the ECOS block (telling ION to forward the bundle on all available routes).

bpcancel operates by invoking findBundle to locate the bundle you want to cancel. findBundle searches a hashtable of BundleSets for this purpose (rather than doing an exhaustive search of the bundle store). The purpose of the BundleSet structure is to minimize bundle storage consumption by just retaining one copy of the bundle payload and reference-counting it, regardless of the number of copies of the bundle encapsulating that payload.

When there is always only a single copy of the bundle, the address of that one copy is recorded inside the BundleSet structure and bpcancel will work great: findBundle will return the address of the bundle so that bpDestroyBundle can destroy it. But when there are multiple copies, the bundle has no single address. We could manage a list of all the copies inside the BundleSet, but that seemed like potentially a lot of overhead and there didn't seem to be any significant requirement for that bookkeeping. So when we clone multiple copies of the bundle, we just set the BundleSet's bundleObj to zero to indicate the bundle's multiplicity.

It's not possible, in general, to cancel the transmission of a bundle for which -- at one time, at least -- there were multiple copies, because one or more of those copies may already have been transmitted. I'm pretty sure that's why you were seeing that strange behavior. Transmitting a copy of a cloned bundle may reduce the reference count in its BundleSet to 1, but it won't insert a new value for bundleObj and you still won't be able to cancel the transmission of that bundle. (That is, bset->bundleObj will be 0 if count > 1, but not only if count > 1.)

The remaining copy (or copies) of the original cloned bundle will eventually disappear in any case, either because they finally get transmitted or because they time out. But you can't get rid of them with bpcancel.

Finally, the check on the return value from findBundle in bpcancel.c is correct. That return value is -1 in the event of some serious system error, which is all the code is looking for; the reference count from the BundleSet doesn't really provide any information that bpcancel cares about. The return value that is actually useful to bpcancel is the bundle address in bundleObj, which it checks: if that is non-zero then the bundle is unique and still in storage, and can be destroyed; if not, not.

[Scott Burleigh]

Addressed by a misc-v3.2.2 change set.