On Mon, Oct 04, 2021 at 09:24:31AM +0200, Miroslav Lichvar wrote: > On Sat, Oct 02, 2021 at 07:09:02AM -0700, Richard Cochran wrote: > > The function, clock_time_properties, is a query that should not change > > the program state. > > So, would you like to move it to a new function, e.g. > clock_update_time_properties()?
Yes, that would be more clear. > > With this change, one call chain will be: > > > > port_tx_announce > > clock_time_properties > > clock_update_utc_offset > > clock_update_grandmaster > > > > That clobbers the grandmasterIdentity with the local dds.clockIdentity > > in case of BC. > > The function specifically checks for grandmaster: > > > > + /* Don't do anything if not a grandmaster with a leap flag set. */ > > > + if (c->cur.stepsRemoved > 0 || leap == 0) { > > > + return; > > > + } > > > + > > If it's a BC, stepsRemoved should be larger than zero, right? I > thought that was easier than looking for a port in the slave state. I see. Still this bothers me. Global functions that query shouldn't have side effects. In C++ one would mark clock_time_properties() as const. The logic that needs the LS flags cleared (Tx Announce) should do that explicitly. How about this? 1. Refactor the setting of clock.tds into a common helper function, as you suggest. 2. Provide a global function to clear the LS flags, called from port_tx_announce explicitly. This could be clock_update_utc_offset(), but the name might suggest that the clock would be stepped. Maybe clock_clear_leap_flags() instead? 3. And BTW, the naming of clock_update_grandmaster() isn't great. Long ago I couldn't think of a better name at the time. It sounds like it would update something about a remote GM. Rename clock_update_grandmaster() to clock_reset_local_gm(). Thoughts? Thanks, Richard _______________________________________________ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel