On Sat, 19 Jul 2014 09:33:38 +0200, Richard Cochran wrote:
> Sorry, just getting around to this. My general impression is that the
> series looks good. Two comments below, but neither are show stoppers.
Thanks. Sorry for the long delay, this time on my part because of summer
vacation and all things that piled up while I was off.
> > +struct port {
> > + LIST_ENTRY(port) list;
> > +};
>
> I almost objected to this, as a matter of principle, because it is
> more clean to have truly opaque objects. But on second thought, adding
> a wrapper around the port is extra busy work (malloc/free) without
> much benefit. So this is fine as is, but let's not get into the habit
> of "friend" classes, if you know what I mean.
Yes. I don't like it myself but I was not able to come up with anything
nicer. The most clean way would be to define
struct port_public {
LIST_ENTRY(port_public) list;
};
and modify struct public to look like:
struct port {
struct port_public public;
char *name;
struct clock *clock;
...
};
This is highly unpractical, though.
> > +static void clock_remove_port(struct clock *c, struct port *p)
> > +{
> > + /* Do not bother with pollfd resizing, we don't mind if it's a bit
> > + * larger than needed and clock_destroy takes care of freeing it in
> > + * case we're shutting down. */
>
> This comment is confusing. The only caller of clock_remove_port is
> clock_destroy anyhow.
Okay, fair enough.
> Should there be more callers one day, then to me
> it is more important to know that port_close sets all of the port's
> FDs to -1.
I'm not sure I follow. port_close destroys the port structure
completely. As part of the port removal, number of ports is decreased,
so pollfd contains always only fds for those ports that are present. No
-1's for removed ports.
Jiri
--
Jiri Benc
------------------------------------------------------------------------------
_______________________________________________
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
