ubus file descriptor passing and channel support #263
Conversation
nbd168
changed the title
|
@jow-, any comments? |
There was a problem hiding this comment.
Hi,
sorry it took me a while to review this. Find my comments below.
I wanted to hold it back until I refactored the ubus module to get rid of global variables (similar to 63e18ea) but I think we should get this in first, then refactor.
lib/ubus.c
Outdated
| "method", UC_STRING, REQUIRED, &funname, | ||
| "data", UC_OBJECT, OPTIONAL, &funargs, | ||
| "cb", UC_CLOSURE, OPTIONAL, &replycb, | ||
| "fd", UC_INTEGER, NAMED, &fd, |
There was a problem hiding this comment.
To align with he behavior of other library functions dealing with descriptors, it would be good if this function would take any handle-like object, that is either a plain integer fd (>= 0) or any object implementing a fileno() method. See get_fd() in socket.c as example.
Since such "wrapped descriptors" are usually tied to the container object lifetime (e.g. GC'ing the handle object will close() the underlying descriptor) it is likely needed to retain a reference to the handle value - at least in the case of deferred requests where the ubus call context outlives the library function call.
There was a problem hiding this comment.
Makes sense. I will implement all of those suggestions today.
On the topic of GC - what do you think about extending uc_resource_t to contain a gc_mark function callback?
That would allow resource owners to simply call ucv_set_mark on all of their references instead of having to manage a registry array manually.
If you agree with that approach, I can make a PR for it soon.
There was a problem hiding this comment.
That would allow resource owners to simply call ucv_set_mark on all of their references instead of having to manage a registry array manually.
The mark flag is also used for other purposes, e.g. to detect circular structures when serializing a value to a string representation, so "pre marking" resource values could interfere and lead to spurious circular reference exceptions.
We could utilize the u64_or_constant header flag though. It is unused for non-integer, non-array, non-object values. When set on a UC_RESOURCE, it could instruct the gc to consider that resource value marked, effectively exempting it from reachability-based garbage collection decisions while the ordinary refcount semantics would still apply.
diff --git a/include/ucode/types.h b/include/ucode/types.h
index f149b98..8e279c9 100644
--- a/include/ucode/types.h
+++ b/include/ucode/types.h
@@ -443,6 +443,26 @@ ucv_resource_create(uc_vm_t *vm, const char *type, void *value)
return ucv_resource_new(t, value);
}
+static inline bool
+ucv_resource_is_external(uc_value_t *uv)
+{
+ return (((uintptr_t)uv & 3) == 0 && uv != NULL &&
+ uv->u64_or_constant == true && uv->type == UC_RESOURCE);
+}
+
+static inline bool
+ucv_resource_set_external(uc_value_t *uv, bool external)
+{
+ if (((uintptr_t)uv & 3) == 0 && uv != NULL &&
+ uv->u64_or_constant != external && uv->type == UC_RESOURCE) {
+ uv->u64_or_constant = external;
+
+ return true;
+ }
+
+ return false;
+}
+
uc_value_t *ucv_regexp_new(const char *, bool, bool, bool, char **);
uc_value_t *ucv_upvalref_new(size_t);
diff --git a/types.c b/types.c
index 5dbf6a8..e584533 100644
--- a/types.c
+++ b/types.c
@@ -2441,7 +2441,7 @@ ucv_gc_common(uc_vm_t *vm, bool final)
if (ucv_is_marked(val))
ucv_clear_mark(val);
- else
+ else if (!ucv_resource_is_external(val))
ucv_free(val, true);
}
There was a problem hiding this comment.
I didn't mean pre-marking resource values, and I'm mostly interested in dealing with non-resource values held by a resource.
What I had in mind is this:
ucv_gc_mark encounters a resource, calls the gc_mark function pointer from the resource type. That function calls ucv_set_mark on all values that the resource holds references to (via uc_value_t pointers in its struct). Unless I'm misreading something, that should preserve the existing semantics of how mark is used.
There was a problem hiding this comment.
Untested patch: https://nbd.name/p/50e647fb
When a function supports many parameters, the order can be somewhat
confusing, especially when dealing with several optional ones.
In order to make this easier to use, support for passing an object with
named parameters.
for example:
obj.notify("test", data, null, null, null, 1000);
can be written as:
obj.notify({
method: "test",
data,
timeout: 1000
});
Signed-off-by: Felix Fietkau <[email protected]>
This can be used to synchronously complete a deferred ubus request, waiting for completion or timeout. Signed-off-by: Felix Fietkau <[email protected]>
This can be used to send and receive file descriptors from within an object method call. Signed-off-by: Felix Fietkau <[email protected]>
File descriptors can be passed via the named fd argument Signed-off-by: Felix Fietkau <[email protected]>
Add a named parameter fd_cb, which is called when the callee returned a file descriptor. Signed-off-by: Felix Fietkau <[email protected]>
nbd168
force-pushed
the
ubus-fd
branch
2 times, most recently
from
8765992 to
23ec0e2
Compare
A channel is a context that is directly connected to a peer instead of going through ubusd. The use of this context is limited to calling ubus_invoke and receiving requests not bound to any registered object. The main use case for this is having a more stateful interaction between processes. A service using channels can attach metadata to each individual channel and keep track of its lifetime, which is not possible through the regular subscribe/notify mechanism. Using channels also improves request latency, since messages are passed directly between processes. A channel can either be opened by fd using ubus.open_channel(), or created from within a request by using req.new_channel(). When calling req.new_channel, the fd for the other side of the channel is automatically passed to the remote caller. Signed-off-by: Felix Fietkau <[email protected]>
|
Should be ready for merge now |
|
Merged, thanks! |
Happy new year!
These changes implement support for the various ways of passing file descriptors via ubus, as well as opening channels.