The attached patch implements introspecting the sbus interfaces as tracked by #2234.
There is one part of the patch I dislike, but I wanted to get other opinions, too -- the discard_const in sbus_message_handler(). I was going back and forth on whether sbus_introspect() should be an sbus handler like the other handlers or whether I should special case it (and then pass the interface directly and not through a void pointer). Special casing the introspection might be a bit cleaner, but would cause some duplication in sbus_message_handler().
The other question I have is whether there should be a way to mark an interface as not introspectable? I can't think of a reason, so by default all interfaces can be introspected.
The introspection also doesn't generate DocString annotations yet, I will send a patch for that separately.
On 04/10/2014 11:03 AM, Jakub Hrozek wrote:
The attached patch implements introspecting the sbus interfaces as tracked by #2234.
There is one part of the patch I dislike, but I wanted to get other opinions, too -- the discard_const in sbus_message_handler(). I was going back and forth on whether sbus_introspect() should be an sbus handler like the other handlers or whether I should special case it (and then pass the interface directly and not through a void pointer). Special casing the introspection might be a bit cleaner, but would cause some duplication in sbus_message_handler().
In my opinion it is correct to make it a standard sbus handler. I think that it is ok to use discard_const() in this situation, however it can be avoided. Cleaner solution would be to create a new context for private data, e.g.: struct sbus_introspect_ctx { const struct sbus_interface_meta *iface; };
The other question I have is whether there should be a way to mark an interface as not introspectable? I can't think of a reason, so by default all interfaces can be introspected.
We should deal with this question when we actually hit an interface that must not be introspectable for some reason.
The introspection also doesn't generate DocString annotations yet, I will send a patch for that separately.
The code looks good and it works splendidly! Good job!
I'm leaving the discard_const issue on your decision, I don't think it is really necessary to avoid it in this case.
It's an ack from me.
On Fri, Apr 11, 2014 at 12:53:08PM +0200, Pavel Březina wrote:
On 04/10/2014 11:03 AM, Jakub Hrozek wrote:
The attached patch implements introspecting the sbus interfaces as tracked by #2234.
There is one part of the patch I dislike, but I wanted to get other opinions, too -- the discard_const in sbus_message_handler(). I was going back and forth on whether sbus_introspect() should be an sbus handler like the other handlers or whether I should special case it (and then pass the interface directly and not through a void pointer). Special casing the introspection might be a bit cleaner, but would cause some duplication in sbus_message_handler().
In my opinion it is correct to make it a standard sbus handler. I think that it is ok to use discard_const() in this situation, however it can be avoided. Cleaner solution would be to create a new context for private data, e.g.: struct sbus_introspect_ctx { const struct sbus_interface_meta *iface; };
The other question I have is whether there should be a way to mark an interface as not introspectable? I can't think of a reason, so by default all interfaces can be introspected.
We should deal with this question when we actually hit an interface that must not be introspectable for some reason.
The introspection also doesn't generate DocString annotations yet, I will send a patch for that separately.
The code looks good and it works splendidly! Good job!
I'm leaving the discard_const issue on your decision, I don't think it is really necessary to avoid it in this case.
It's an ack from me.
Hi,
I think the sbus_introspect_ctx context is a good idea. It requires some special casing in sbus_message_handler(), though..
Attached are two patches, one is the modified patch including the sbus_introspect_ctx, the other is just the interdiff for easier review.
Thanks for the review!
On 04/14/2014 04:17 PM, Jakub Hrozek wrote:
On Fri, Apr 11, 2014 at 12:53:08PM +0200, Pavel Březina wrote:
On 04/10/2014 11:03 AM, Jakub Hrozek wrote:
The attached patch implements introspecting the sbus interfaces as tracked by #2234.
There is one part of the patch I dislike, but I wanted to get other opinions, too -- the discard_const in sbus_message_handler(). I was going back and forth on whether sbus_introspect() should be an sbus handler like the other handlers or whether I should special case it (and then pass the interface directly and not through a void pointer). Special casing the introspection might be a bit cleaner, but would cause some duplication in sbus_message_handler().
In my opinion it is correct to make it a standard sbus handler. I think that it is ok to use discard_const() in this situation, however it can be avoided. Cleaner solution would be to create a new context for private data, e.g.: struct sbus_introspect_ctx { const struct sbus_interface_meta *iface; };
The other question I have is whether there should be a way to mark an interface as not introspectable? I can't think of a reason, so by default all interfaces can be introspected.
We should deal with this question when we actually hit an interface that must not be introspectable for some reason.
The introspection also doesn't generate DocString annotations yet, I will send a patch for that separately.
The code looks good and it works splendidly! Good job!
I'm leaving the discard_const issue on your decision, I don't think it is really necessary to avoid it in this case.
It's an ack from me.
Hi,
I think the sbus_introspect_ctx context is a good idea. It requires some special casing in sbus_message_handler(), though..
Attached are two patches, one is the modified patch including the sbus_introspect_ctx, the other is just the interdiff for easier review.
Thanks for the review!
Ack.
sssd-devel@lists.fedorahosted.org
-
Jakub Hrozek -
Pavel Březina