11:14 a.m.
---
ell/netlink.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/ell/netlink.c b/ell/netlink.c
index 33d0e5b..4664762 100644
--- a/ell/netlink.c
+++ b/ell/netlink.c
@@ -162,12 +162,15 @@ static void process_message(struct l_netlink *netlink, struct
nlmsghdr *nlmsg)
{
const void *data = nlmsg;
struct command *command;
+ uint32_t id;
command = l_hashmap_remove(netlink->command_pending,
L_UINT_TO_PTR(nlmsg->nlmsg_seq));
if (!command)
return;
+ id = command->id;
+
if (!command->handler)
goto done;
@@ -189,9 +192,8 @@ static void process_message(struct l_netlink *netlink, struct nlmsghdr
*nlmsg)
}
done:
- l_hashmap_remove(netlink->command_lookup, L_UINT_TO_PTR(command->id));
-
- destroy_command(command);
+ if (l_hashmap_remove(netlink->command_lookup, L_UINT_TO_PTR(id)))
+ destroy_command(command);
}
static void process_multi(struct l_netlink *netlink, struct nlmsghdr *nlmsg)
--
2.27.0
3:10 p.m.
Hi Andrew,
On 3/2/21 5:14 AM, Andrew Zaborowski wrote:
---
ell/netlink.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
You may want to write something about why this would be wanted.
diff --git a/ell/netlink.c b/ell/netlink.c
index 33d0e5b..4664762 100644
--- a/ell/netlink.c
+++ b/ell/netlink.c
@@ -162,12 +162,15 @@ static void process_message(struct l_netlink *netlink, struct
nlmsghdr *nlmsg)
{
const void *data = nlmsg;
struct command *command;
+ uint32_t id;
command = l_hashmap_remove(netlink->command_pending,
L_UINT_TO_PTR(nlmsg->nlmsg_seq));
if (!command)
return;
+ id = command->id;
+
if (!command->handler)
goto done;
@@ -189,9 +192,8 @@ static void process_message(struct l_netlink *netlink, struct
nlmsghdr *nlmsg)
}
done:
- l_hashmap_remove(netlink->command_lookup, L_UINT_TO_PTR(command->id));
-
- destroy_command(command);
+ if (l_hashmap_remove(netlink->command_lookup, L_UINT_TO_PTR(id)))
+ destroy_command(command);
Would it not be better to simply remove the command from the command_lookup
hashmap prior to dispatch, and have l_netlink_cancel simply fail?
I do wonder why you'd cancel the command from the handler?
}
static void process_multi(struct l_netlink *netlink, struct nlmsghdr *nlmsg)
Regards,
-Denis
3:34 p.m.
On Tue, 2 Mar 2021 at 16:10, Denis Kenzior <denkenz(a)gmail.com> wrote:
On 3/2/21 5:14 AM, Andrew Zaborowski wrote:
> ---
> ell/netlink.c | 8 +++++---
> 1 file changed, 5 insertions(+), 3 deletions(-)
>
You may want to write something about why this would be wanted.
Ok.
> diff --git a/ell/netlink.c b/ell/netlink.c
> index 33d0e5b..4664762 100644
> --- a/ell/netlink.c
> +++ b/ell/netlink.c
> @@ -162,12 +162,15 @@ static void process_message(struct l_netlink *netlink, struct
nlmsghdr *nlmsg)
> {
> const void *data = nlmsg;
> struct command *command;
> + uint32_t id;
>
> command = l_hashmap_remove(netlink->command_pending,
> L_UINT_TO_PTR(nlmsg->nlmsg_seq));
> if (!command)
> return;
>
> + id = command->id;
> +
> if (!command->handler)
> goto done;
>
> @@ -189,9 +192,8 @@ static void process_message(struct l_netlink *netlink, struct
nlmsghdr *nlmsg)
> }
>
> done:
> - l_hashmap_remove(netlink->command_lookup, L_UINT_TO_PTR(command->id));
> -
> - destroy_command(command);
> + if (l_hashmap_remove(netlink->command_lookup, L_UINT_TO_PTR(id)))
> + destroy_command(command);
Would it not be better to simply remove the command from the command_lookup
hashmap prior to dispatch, and have l_netlink_cancel simply fail?
Yes, that would work.
I do wonder why you'd cancel the command from the handler?
In the IWD AP case I want to cancel a dump command which I think is
already handled correctly, but if the API can be made more robust then
I think it's an improvement. There can be complicated code paths
where you destroy an object that owns the netlink command as a result
of the command response.
Best regards
4:35 p.m.
Hi Andrew,
> I do wonder why you'd cancel the command from the handler?
In the IWD AP case I want to cancel a dump command which I think is
already handled correctly, but if the API can be made more robust then
I think it's an improvement. There can be complicated code paths
where you destroy an object that owns the netlink command as a result
of the command response.
Sure, I buy the more robust argument, though I still wonder how one would even
get into such a state on purpose. But the last sentence seems to be handwaving?
So did you somehow trigger this condition or just fixing ghost bugs?
Regards,
-Denis
9:30 p.m.
On Tue, 2 Mar 2021 at 17:35, Denis Kenzior <denkenz(a)gmail.com> wrote:
> In the IWD AP case I want to cancel a dump command which I think
is
> already handled correctly, but if the API can be made more robust then
> I think it's an improvement. There can be complicated code paths
> where you destroy an object that owns the netlink command as a result
> of the command response.
Sure, I buy the more robust argument, though I still wonder how one would even
get into such a state on purpose. But the last sentence seems to be handwaving?
Well, in the AP code we're actually careful to reset e.g.
ap->rtnl_add_cmd to 0 at the beginning of the reply handler but if we
were resetting it any later we'd be affected by this.
So did you somehow trigger this condition or just fixing ghost
bugs?
I was only checking the code to see if it was safe to cancel the dump
command the way I did it in my IWD patchset. It seemed to be safe for
dump commands but not for regular commands.
The normal sequence is this:
1. l_netlink_send call
2. handler callback
3. destroy callback
After 3. I'd consider it wrong to call l_netlink_cancel() with the
original ID because you'd be risking cancelling an unrelated command
(there's no guarantee that the ID wasn't re-used since), but until 3.
I think it should be made safe.
Best regards
11:29 p.m.
Hi Andrew
On 3/2/21 3:30 PM, Andrew Zaborowski wrote:
On Tue, 2 Mar 2021 at 17:35, Denis Kenzior <denkenz(a)gmail.com>
wrote:
>> In the IWD AP case I want to cancel a dump command which I think is
>> already handled correctly, but if the API can be made more robust then
>> I think it's an improvement. There can be complicated code paths
>> where you destroy an object that owns the netlink command as a result
>> of the command response.
>
> Sure, I buy the more robust argument, though I still wonder how one would even
> get into such a state on purpose. But the last sentence seems to be handwaving?
Well, in the AP code we're actually careful to reset e.g.
ap->rtnl_add_cmd to 0 at the beginning of the reply handler but if we
were resetting it any later we'd be affected by this.
Hmm... This is pretty typical pattern for when one doesn't provide a destroy
callback. I'm not sure I understand what you're trying to convey here...?
Are you trying to stop callbacks mid-dump or ?
> So did you somehow trigger this condition or just fixing ghost
bugs?
I was only checking the code to see if it was safe to cancel the dump
command the way I did it in my IWD patchset. It seemed to be safe for
dump commands but not for regular commands.
Yes, but *why* would you use l_netlink_cancel for the command when you're
already in the handler for that exact same command?
The normal sequence is this:
1. l_netlink_send call
2. handler callback
3. destroy callback
After 3. I'd consider it wrong to call l_netlink_cancel() with the
original ID because you'd be risking cancelling an unrelated command
(there's no guarantee that the ID wasn't re-used since), but until 3.
I think it should be made safe.
I can't really imagine why anyone would invoke l_netlink_cancel in a callback.
Not saying we shouldn't allow this, but the use case is still a mystery to me?
Also, we sort of cheat in many places and assume that only a single message will
be sent for certain commands. Underneath, there's no real difference between a
dump and a non-dump. In fact the kernel might (someday) choose to send multiple
messages as a reply. The only way you know that you're done (for netlink/genl
at least) is after the destroy callback has been called. The callback itself
has no way of knowing whether it is a single-part or multi-part message...
Regards,
-Denis
12:02 a.m.
On Wed, 3 Mar 2021 at 00:29, Denis Kenzior <denkenz(a)gmail.com> wrote:
On 3/2/21 3:30 PM, Andrew Zaborowski wrote:
> On Tue, 2 Mar 2021 at 17:35, Denis Kenzior <denkenz(a)gmail.com> wrote:
>>> In the IWD AP case I want to cancel a dump command which I think is
>>> already handled correctly, but if the API can be made more robust then
>>> I think it's an improvement. There can be complicated code paths
>>> where you destroy an object that owns the netlink command as a result
>>> of the command response.
>>
>> Sure, I buy the more robust argument, though I still wonder how one would even
>> get into such a state on purpose. But the last sentence seems to be
handwaving?
>
> Well, in the AP code we're actually careful to reset e.g.
> ap->rtnl_add_cmd to 0 at the beginning of the reply handler but if we
> were resetting it any later we'd be affected by this.
>
Hmm... This is pretty typical pattern for when one doesn't provide a destroy
callback. I'm not sure I understand what you're trying to convey here...?
Are you trying to stop callbacks mid-dump or ?
Yes, in the dump case the use case is pretty obvious -- once you've
received the fragment you were interested in (in my case, I dump IPv4
addresses and I'm interested in one specific ifindex), if you can
cancel the command you don't have to keep a flag that tells you to
ignore further callbacks. And if you need to destroy the object (e.g.
stop the AP) based on the data you received, you're also free to do
this.
In the non-dump case I see the utility of this change mainly in the
situation where, within the callback, you decide you want to tear-down
the object (e.g. stop the AP), like I said already. If you're not
allowed to cancel the command at that point, you basically need
reference counting so that you can avoid crashing in your destroy
handler. And in that case I'd say we should have at least a warning
about this limitation somewhere.
>> So did you somehow trigger this condition or just fixing ghost bugs?
>
> I was only checking the code to see if it was safe to cancel the dump
> command the way I did it in my IWD patchset. It seemed to be safe for
> dump commands but not for regular commands.
Yes, but *why* would you use l_netlink_cancel for the command when you're
already in the handler for that exact same command?
>
> The normal sequence is this:
> 1. l_netlink_send call
> 2. handler callback
> 3. destroy callback
>
> After 3. I'd consider it wrong to call l_netlink_cancel() with the
> original ID because you'd be risking cancelling an unrelated command
> (there's no guarantee that the ID wasn't re-used since), but until 3.
> I think it should be made safe.
I can't really imagine why anyone would invoke l_netlink_cancel in a callback.
Not saying we shouldn't allow this, but the use case is still a mystery to me?
Also, we sort of cheat in many places and assume that only a single message will
be sent for certain commands. Underneath, there's no real difference between a
dump and a non-dump. In fact the kernel might (someday) choose to send multiple
messages as a reply. The only way you know that you're done (for netlink/genl
at least) is after the destroy callback has been called. The callback itself
has no way of knowing whether it is a single-part or multi-part message...
Oh right, the dump/non-dump distinction in my example is based on the
current rtnl behavior.
Best regards
12:18 a.m.
Hi Andrew,
> Are you trying to stop callbacks mid-dump or ?
Yes, in the dump case the use case is pretty obvious -- once you've
received the fragment you were interested in (in my case, I dump IPv4
addresses and I'm interested in one specific ifindex), if you can
cancel the command you don't have to keep a flag that tells you to
ignore further callbacks. And if you need to destroy the object (e.g.
stop the AP) based on the data you received, you're also free to do
this.
Ok, gotcha. I have to ponder whether using l_foo_cancel is a good idea for
this, or a more explicit API should be used. Also, we'd have to make this
behavior consistent everywhere.
In the non-dump case I see the utility of this change mainly in the
situation where, within the callback, you decide you want to tear-down
the object (e.g. stop the AP), like I said already. If you're not
allowed to cancel the command at that point, you basically need
reference counting so that you can avoid crashing in your destroy
handler. And in that case I'd say we should have at least a warning
about this limitation somewhere.
You might be the first one to need this actually. There's always the
l_idle_oneshot get-out-of-jail card ;)
Regards,
-Denis
9:36 p.m.
Hi Denis,
On Tue, 2 Mar 2021 at 16:34, Andrew Zaborowski
<andrew.zaborowski(a)intel.com> wrote:
On Tue, 2 Mar 2021 at 16:10, Denis Kenzior <denkenz(a)gmail.com>
wrote:
> > done:
> > - l_hashmap_remove(netlink->command_lookup,
L_UINT_TO_PTR(command->id));
> > -
> > - destroy_command(command);
> > + if (l_hashmap_remove(netlink->command_lookup, L_UINT_TO_PTR(id)))
> > + destroy_command(command);
>
> Would it not be better to simply remove the command from the command_lookup
> hashmap prior to dispatch, and have l_netlink_cancel simply fail?
Yes, that would work.
On a second thought, I think it's better the other way. The
l_netlink_cancel() call can be used as a request to forget the
callbacks. The user may be tearing down the object they passed as
user_data for example. IIRC we did take that use case into
consideration in some of the other APIs, what do you think?
>
> >
> > I do wonder why you'd cancel the command from the handler?
>
> In the IWD AP case I want to cancel a dump command which I think is
> already handled correctly, but if the API can be made more robust then
> I think it's an improvement. There can be complicated code paths
> where you destroy an object that owns the netlink command as a result
> of the command response.
>
> Best regards
12:03 a.m.
Hi Andrew,
>> Would it not be better to simply remove the command from the
command_lookup
>> hashmap prior to dispatch, and have l_netlink_cancel simply fail?
>
> Yes, that would work.
On a second thought, I think it's better the other way. The
l_netlink_cancel() call can be used as a request to forget the
callbacks. The user may be tearing down the object they passed as
user_data for example. IIRC we did take that use case into
consideration in some of the other APIs, what do you think?
Which other way? Ahhh, I think I get it... You want to unset the destroy
callback or prevent it from being called subsequently when inside the handler by
issuing l_netlink_cancel?
I actually don't think any ell APIs work this way. l_dbus for example simply
removes the pending request prior to dispatch (which is what I suggested
earlier). Hence l_dbus_cancel wouldn't work on it inside the l_dbus_send callback.
Even APIs that use the watchlist pattern don't do it this way. You may be
thinking of the scan API inside iwd, which did it for a different reason.
Regards,
-Denis
533
days inactive
534
days old
9 comments
2 participants
participants (2)
-
Andrew Zaborowski -
Denis Kenzior