On Thu, Apr 28, 2011 at 01:13:21PM +0200, Marc Kleine-Budde wrote:
> On 04/28/2011 12:55 PM, Kurt Van Dijck wrote:
> > On Thu, Apr 28, 2011 at 09:45:30AM +0200, Marc Kleine-Budde wrote:
> >>> +LDADD += libj1939.la
> >>
> >> It would be cleaner if you just link your j1939 programs with the lib:
> >>
> >> jacd_LDADD = libj1939.la
> >> jsr_LDADD = libj1939.la
> >> jspy_LDADD = libj1939.la
> >>
> >> (I know we link everything against libcan.la)
> >
> > you're right. Thanks for the example automake syntax!
> > ---
> > This patch adds SAE J1939 tools & libraries to can-utils
> >
> > * jacd: a J1939 address claiming daemon
> > * jspy: spy on a J1939 bus
> > * jsr: send/receive J1939 packets
> >
> > * libj1939.a: conversion to/from struct sockaddr_can to string
> >
> > Changes with v1:
> > * update for autotools
> >
> > Signed-off-by: Kurt Van Dijck <[email protected]>
>
> I quickly looked over the code, see some comments inline.
>
Thanks, I have some additional questions:
> > ---
> > + /* run */
> > + while (1) {
> > + ret = poll(pfd, 2, -1);
> ARRAY_SIZE?
is that available in userspace? cool!
> > + if (ret < 0)
> > + error(1, errno, "poll()");
>
> what about EINTR?
>
well, that's indeed a typical one, but the remainder of the program is
not interested in any signal...
> > + break;
> > + }
> > + len = ret;
> > + for (done = 0; done < len; ) {
> > + ret = send(pfd[1].fd, buf, len, s.sendflags);
> > + if (ret < 0) {
> > + error(0, errno, "write(%s)",
> > libj1939_addr2str(&s.src));
> > + if (ENOBUFS == errno) {
>
> the other way round?
>
???
> > + sleep(1);
> > + continue;
> > + }
> > + exit(1);
> > + }
> > + done += ret;
> > + }
> > + }
> > + if (pfd[1].revents) {
> > + ret = read(pfd[1].fd, buf, sizeof(buf));
> > + if (ret < 0) {
> > + ret = errno;
> > + error(0, errno, "read(%s)",
> > libj1939_addr2str(&s.dst));
> > + switch (ret) {
> > + case EHOSTDOWN:
> > + break;
> > + default:
> > + exit(1);
> > + }
> > + } else {
> > + write(STDOUT_FILENO, buf, ret);
> > + }
> > + }
> > + }
> > +
> > + free(buf);
> close()?
will close 1 statement further...
> > + return 0;
> > +}
closed!
is that good enough?
> > +//-----------------------------------------------------------------------------
> > +
> > +//-----------------------------------------------------------------------------
> > +static struct ifname *libj1939_add_ifnam(int ifindex, const char *str)
> > +{
> > + struct ifname *nam;
> > + nam = malloc(sizeof(*nam) + strlen(str));
> > + memset(nam, 0, sizeof(*nam));
> calloc?
well, that's possible. I tend to not use calloc, but that's personal.
In fact, you're right here.
> > + nam->ifindex = ifindex;
> > + strcpy(nam->name, str);
> > + nam->next = s.names;
> > + s.names = nam;
> > + return nam;
> > +}
> > +
Kurt
_______________________________________________
Socketcan-core mailing list
[email protected]
https://lists.berlios.de/mailman/listinfo/socketcan-core
