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

[Marcel Holtmann <marcel@holtmann.org>]

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

Hi Sjur,

> > > +static GSList *pending_requests;
> > > +static GIOChannel *channel;
> > > +static guint32 rtnl_seqnr;
> > > +static guint  rtnl_watch;
> > 
> > To be fair, you don't need both, the GIOChannel and the watch. You can
> > just add the watch and then unref the GIOChannel and set it to close on
> > unref. That way when the watch gets removed or you remove it, the
> > GIOChannel and the underlaying socket will be automatically closed as
> > well.
> ....
> > As described above. Just unref the GIOChannel and forget about it. And
> > then you can make the GIOChannel a local variable.
> 
> I think I have to keep the channel, because I do need the
> file descriptor derived from the channel when doing sendto.

fair enough. Anything wrong with just using g_io_channel_write_chars and
setting the channel to not-buffered and no-encoding?

> > > > 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.
> ...
> > You don't need to append it first, send the message, check error and in
> > case of an error remove it again.
> 
> OK, thanks - now I get what your after. I completely misunderstood your 
> previous comment, my bad.

That is what I figured. I think that I wasn't clear enough here.

> > 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.
> 
> So I'll add something like this then:
> 	fl = fcntl(sk, F_GETFL);
> 	fcntl(sk, F_SETFL, fl | O_NONBLOCK);

You can also just use the one from GIOChannel to do this since you wrap
it into a GIOChannel right away anyway.

> > > +	}
> > > +
> > > +	g_slist_free(pending_requests);
> > > +	pending_requests = NULL;
> > 
> > No need to set pending_requests to NULL here.
> 
> But I will have to move this to the caif_rtnl_init function then,
> I think pending_requests needs to be NULL before using it.

That is given by the fact that it is a global static variable. And when
the exit gets called, we are ending the ofonod.

Not a big thing. Just a minor nitpick. And if you look you will find
cases where even I kept doing it ;)

> > > > 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 got rid of the structs and typedef'ed response function - I agree this better.
> 
> <snip>
> typedef void (*rtnl_create_cb_t) (int result, gpointer user_data,
> 		char *ifname, int ifindex);
> 
> extern int caif_rtnl_create_interface(gpointer user_data, int type, int connid,
> 		gboolean loop, rtnl_create_cb_t cb);
> 
> 
> Hoping to get the next patch out later today.

Please don't use rtnl_ prefix. That is not your namespace. So it should
be caif_rtnl_create_cb.

Regards

Marcel