RE: [PATCHv2 1/2] stemodem: Add RTNL functionality managing CAIF Network Interfaces.

[Marcel Holtmann <marcel@holtmann.org>]

[-- Attachment #1: Type: text/plain, Size: 2770 bytes --]

Hi Sjur,

> > You don't really have to play this append/remove game. You can just
> > append it after send_rtnl_req succeed. The read goes via the mainloop
> > anyway and we are a single threaded program. So the order is
> > guaranteed.
> 
> I'm not sure about this. I need to keep track of what
> connection-id I assign to the CAIF Interface in the request and what 
> interface name and interface index I get for that connection id
> in the RTNL response.
> This mapping between network interface and connection id is used 
> when PDP Contexts are activated. The append/remove game provides me
> with the request-response mapping. 

I know what you are trying to protect here, but what I am saying is that
just appending the struct to the list after you send it (and evaluated
its error code) is just fine.

You don't need to append it first, send the message, check error and in
case of an error remove it again.

The nature of single-thread application using a mainloop and where input
is driven by that mainloop, ensures that you will only receive the reply
to your message after you added it to the list. Even if that is done
after you send it.

So while it is fine to append the structure to the list for further
tracking, you should just do it after you send the message and it
succeeded and be done with it.

And while we are at it. Essentially you also need to use asynchronous
and non-blocking watch driven sending of your message in the first
place. I did forget to check if you open the RTNL socket non-blocking,
but you should be doing that.

> > > +struct rtnl_rsp_param {
> > > +	int result;
> > > +	gpointer user_data;
> > > +	char ifname[IF_NAMESIZE];
> > > +	int  ifindex;
> > > +};
> > > +
> > > +struct rtnl_req_param {
> > > +	void (*callback)(struct rtnl_rsp_param *param);
> > > +	gpointer user_data;
> > > +	int type;
> > > +	int conn_id;
> > > +	gboolean loop_enabled;
> > > +};
> > > +
> > > +extern int caif_rtnl_create_interface(struct rtnl_req_param *req);
> > > +extern int caif_rtnl_delete_interface(int ifid);
> > 
> > I am not really sold on this param stuct thingy btw. Why do you need
> > them? Just proper input params and output params for the callback
> > handler would do them same. Won't they?
> 
> Yes, I could do that. But in that case I would prefer to typedef the 
> response function to simplify create function's signature. Otherwise
> I think the create function's signature gets too complex.

typedef for function callbacks are just fine. I have no problem with
that at all.

I am not set in stone here, but for something this simple, it seems to
me that the two structs is a bit of overhead and complexity that we
could avoid.

Regards

Marcel