Re: [Linuxptp-devel] [PATCH RFC 29/30] interface: Silence warning from gcc version 8.
On 2/11/2020 6:04 AM, Richard Cochran wrote:
> When compiling with gcc8 and -O2, the clever compiler complains:
>
>interface.c: In function ‘interface_ensure_tslabel’:
>interface.c:38:3: error: ‘strncpy’ output may be truncated copying 108
> bytes from a string of length 108 [-Werror=stringop-truncation]
>
> Even though this is a false positive, this patch silences the warning
> by using memcpy instead of strncpy.
>
You could also use snprintf("%s", ..., sizeof(iface->name);
memcpy also copies all of the data even if it doesn't strictly need to.
Thanks,
Jake
> Signed-off-by: Richard Cochran
> ---
> interface.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/interface.c b/interface.c
> index 7cd5b41..2cf3b1e 100644
> --- a/interface.c
> +++ b/interface.c
> @@ -35,7 +35,7 @@ void interface_destroy(struct interface *iface)
> void interface_ensure_tslabel(struct interface *iface)
> {
> if (!iface->ts_label[0]) {
> - strncpy(iface->ts_label, iface->name, MAX_IFNAME_SIZE);
> + memcpy(iface->ts_label, iface->name, MAX_IFNAME_SIZE);
> }
> }
>
>
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH RFC 27/30] pmc: Use the proper create/destroy API for network interfaces.
On 2/11/2020 6:04 AM, Richard Cochran wrote:
> Signed-off-by: Richard Cochran
Reviewed-by: Jacob Keller
> ---
> pmc_common.c | 19 ---
> 1 file changed, 12 insertions(+), 7 deletions(-)
>
> diff --git a/pmc_common.c b/pmc_common.c
> index 41181fb..3aab4b9 100644
> --- a/pmc_common.c
> +++ b/pmc_common.c
> @@ -313,6 +313,7 @@ struct pmc {
> struct PortIdentity target;
>
> struct transport *transport;
> + struct interface *iface;
> struct fdarray fdarray;
> int zero_length_gets;
> };
> @@ -322,11 +323,8 @@ struct pmc *pmc_create(struct config *cfg, enum
> transport_type transport_type,
> UInteger8 domain_number, UInteger8 transport_specific,
> int zero_datalen)
> {
> - struct interface iface;
> struct pmc *pmc;
>
> - memset(, 0, sizeof(iface));
> -
> pmc = calloc(1, sizeof *pmc);
> if (!pmc)
> return NULL;
> @@ -350,18 +348,24 @@ struct pmc *pmc_create(struct config *cfg, enum
> transport_type transport_type,
> goto failed;
> }
>
> - interface_set_name(, iface_name);
> - interface_ensure_tslabel();
> + pmc->iface = interface_create(iface_name);
> + if (!pmc->iface) {
> + pr_err("failed to create interface");
> + goto failed;
> + }
> + interface_ensure_tslabel(pmc->iface);
>
> - if (transport_open(pmc->transport, ,
> + if (transport_open(pmc->transport, pmc->iface,
> >fdarray, TS_SOFTWARE)) {
> pr_err("failed to open transport");
> - goto failed;
> + goto no_trans_open;
> }
> pmc->zero_length_gets = zero_datalen ? 1 : 0;
>
> return pmc;
>
> +no_trans_open:
> + interface_destroy(pmc->iface);
> failed:
> if (pmc->transport)
> transport_destroy(pmc->transport);
> @@ -372,6 +376,7 @@ failed:
> void pmc_destroy(struct pmc *pmc)
> {
> transport_close(pmc->transport, >fdarray);
> + interface_destroy(pmc->iface);
> transport_destroy(pmc->transport);
> free(pmc);
> }
>
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH RFC 26/30] config: Use the proper create/destroy API for network interfaces.
On 2/11/2020 6:04 AM, Richard Cochran wrote:
> Signed-off-by: Richard Cochran
Reviewed-by: Jacob Keller
> ---
> config.c | 6 ++
> 1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/config.c b/config.c
> index 717ee65..e033842 100644
> --- a/config.c
> +++ b/config.c
> @@ -829,13 +829,11 @@ struct interface *config_create_interface(const char
> *name, struct config *cfg)
> return iface;
> }
>
> - iface = calloc(1, sizeof(struct interface));
> + iface = interface_create(name);
> if (!iface) {
> fprintf(stderr, "cannot allocate memory for a port\n");
> return NULL;
> }
> -
> - interface_set_name(iface, name);
> STAILQ_INSERT_TAIL(>interfaces, iface, list);
> cfg->n_interfaces++;
>
> @@ -906,7 +904,7 @@ void config_destroy(struct config *cfg)
>
> while ((iface = STAILQ_FIRST(>interfaces))) {
> STAILQ_REMOVE_HEAD(>interfaces, list);
> - free(iface);
> + interface_destroy(iface);
> }
> while ((table = STAILQ_FIRST(>unicast_master_tables))) {
> while ((address = STAILQ_FIRST(>addrs))) {
>
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH RFC 25/30] clock: Use the proper create/destroy API for network interfaces.
On 2/11/2020 6:04 AM, Richard Cochran wrote:
> Signed-off-by: Richard Cochran
Reviewed-by: Jacob Keller
> ---
> clock.c | 17 +
> 1 file changed, 9 insertions(+), 8 deletions(-)
>
> diff --git a/clock.c b/clock.c
> index 71b5795..e5f104e 100644
> --- a/clock.c
> +++ b/clock.c
> @@ -122,7 +122,7 @@ struct clock {
> struct clock_stats stats;
> int stats_interval;
> struct clockcheck *sanity_check;
> - struct interface uds_interface;
> + struct interface *udsif;> LIST_HEAD(clock_subscribers_head,
> clock_subscriber) subscribers;
> };
>
> @@ -259,6 +259,7 @@ void clock_destroy(struct clock *c)
> {
> struct port *p, *tmp;
>
> + interface_destroy(c->udsif);
> clock_flush_subscriptions(c);
> LIST_FOREACH_SAFE(p, >ports, list, tmp) {
> clock_remove_port(c, p);
> @@ -854,7 +855,7 @@ struct clock *clock_create(enum clock_type type, struct
> config *config,
> const char *uds_ifname;
> struct port *p;
> unsigned char oui[OUI_LEN];
> - struct interface *iface, *udsif = >uds_interface;
> + struct interface *iface;
> struct timespec ts;
> int sfl;
>
> @@ -1003,20 +1004,20 @@ struct clock *clock_create(enum clock_type type,
> struct config *config,
>
> /* Configure the UDS. */
> uds_ifname = config_get_string(config, NULL, "uds_address");
> - interface_set_name(udsif, uds_ifname);
> - if (config_set_section_int(config, interface_name(udsif),
> + c->udsif = interface_create(uds_ifname);
> + if (config_set_section_int(config, interface_name(c->udsif),
> "announceReceiptTimeout", 0)) {
> return NULL;
> }
> - if (config_set_section_int(config, interface_name(udsif),
> + if (config_set_section_int(config, interface_name(c->udsif),
> "delay_mechanism", DM_AUTO)) {
> return NULL;
> }
> - if (config_set_section_int(config, interface_name(udsif),
> + if (config_set_section_int(config, interface_name(c->udsif),
> "network_transport", TRANS_UDS)) {
> return NULL;
> }
> - if (config_set_section_int(config, interface_name(udsif),
> + if (config_set_section_int(config, interface_name(c->udsif),
> "delay_filter_length", 1)) {
> return NULL;
> }
> @@ -1131,7 +1132,7 @@ struct clock *clock_create(enum clock_type type, struct
> config *config,
> }
>
> /* Create the UDS interface. */
> - c->uds_port = port_open(phc_device, phc_index, timestamping, 0, udsif,
> c);
> + c->uds_port = port_open(phc_device, phc_index, timestamping, 0,
> c->udsif, c);
> if (!c->uds_port) {
> pr_err("failed to open the UDS port");
> return NULL;
>
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH RFC 24/30] interface: Introduce methods to create and destroy instances.
On 2/11/2020 6:04 AM, Richard Cochran wrote:
> In order to eventually hide the implementation details of the interface,
> users will need to be able to create and destroy instances thereof. This
> patch adds the needed methods.
>
> Signed-off-by: Richard Cochran
Reviewed-by: Jacob Keller
> ---
> interface.c | 19 +++
> interface.h | 13 +
> 2 files changed, 32 insertions(+)
>
> diff --git a/interface.c b/interface.c
> index 74a2512..63ed7e4 100644
> --- a/interface.c
> +++ b/interface.c
> @@ -4,8 +4,27 @@
> * @note Copyright (C) 2020 Richard Cochran
> * @note SPDX-License-Identifier: GPL-2.0+
> */
> +#include
> #include "interface.h"
>
> +struct interface *interface_create(const char *name)
> +{
> + struct interface *iface;
> +
> + iface = calloc(1, sizeof(struct interface));
Good, calloc guarantees that the interface structure is always zero'd.
That means we don't actually need to worry about interfaces where the
MAS_IFNAME_SIZE arrays end on non-zero. Ok.
I still think it would be good to have the functions guarantee the NULL
by manually assigning or using one of the string copy implementations
that will guarantee it. That way they don't have to rely on this assumption.
> + if (!iface) {
> + return NULL;
> + }
> + interface_set_name(iface, name);
> +
> + return iface;
> +}
> +
> +void interface_destroy(struct interface *iface)
> +{
> + free(iface);
> +}
> +
> void interface_ensure_tslabel(struct interface *iface)
> {
> if (!iface->ts_label[0]) {
> diff --git a/interface.h b/interface.h
> index 32eec7b..b61f4d6 100644
> --- a/interface.h
> +++ b/interface.h
> @@ -25,6 +25,19 @@ struct interface {
> struct sk_ts_info ts_info;
> };
>
> +/**
> + * Creates an instance of an interface.
> + * @param name The device which indentifies this interface.
> + * @return A pointer to an interface instance on success, NULL
> otherwise.
> + */
> +struct interface *interface_create(const char *name);
> +
> +/**
> + * Destroys an instance of an interface.
> + * @param iface A pointer obtained via interface_create().
> + */
> +void interface_destroy(struct interface *iface);
> +
> /**
> * Ensures that an interface has a proper time stamping label.
> * @param iface The interface of interest.
>
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH RFC 23/30] Convert call sites to the proper method for testing time stamping modes.
On 2/11/2020 6:04 AM, Richard Cochran wrote:
> Signed-off-by: Richard Cochran
Reviewed-by: Jacob Keller
> ---
> clock.c | 2 +-
> port.c | 2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/clock.c b/clock.c
> index 845e27a..71b5795 100644
> --- a/clock.c
> +++ b/clock.c
> @@ -957,7 +957,7 @@ struct clock *clock_create(enum clock_type type, struct
> config *config,
> interface_ensure_tslabel(iface);
> interface_get_tsinfo(iface);
> if (interface_tsinfo_valid(iface) &&
> - ((iface->ts_info.so_timestamping & required_modes) !=
> required_modes)) {
This actually makes the line shorter too! Nice.
> + !interface_tsmodes_supported(iface, required_modes)) {
> pr_err("interface '%s' does not support requested
> timestamping mode",
> interface_name(iface));
> return NULL;
> diff --git a/port.c b/port.c
> index b590024..6b87bc9 100644
> --- a/port.c
> +++ b/port.c
> @@ -2514,7 +2514,7 @@ void port_link_status(void *ctx, int linkup, int
> ts_index)
> if (interface_tsinfo_valid(p->iface) &&
> interface_phc_index(p->iface) >= 0) {
> required_modes = clock_required_modes(p->clock);
> - if ((p->iface->ts_info.so_timestamping &
> required_modes) != required_modes) {
> + if (!interface_tsmodes_supported(p->iface,
> required_modes)) {
> pr_err("interface '%s' does not support
> requested "
> "timestamping mode, set link status down
> by force.",
> interface_label(p->iface));
>
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH RFC 22/30] interface: Introduce a method to test supported time stamping modes.
On 2/11/2020 6:04 AM, Richard Cochran wrote:
> Signed-off-by: Richard Cochran
Reviewed-by: Jacob Keller
> ---
> interface.c | 8
> interface.h | 8
> 2 files changed, 16 insertions(+)
>
> diff --git a/interface.c b/interface.c
> index 7a3eddc..74a2512 100644
> --- a/interface.c
> +++ b/interface.c
> @@ -47,3 +47,11 @@ bool interface_tsinfo_valid(struct interface *iface)
> {
> return iface->ts_info.valid ? true : false;
> }
> +
> +bool interface_tsmodes_supported(struct interface *iface, int modes)
> +{
> + if ((iface->ts_info.so_timestamping & modes) == modes) {
> + return true;
> + }
> + return false;
> +}
> diff --git a/interface.h b/interface.h
> index 3526a48..32eec7b 100644
> --- a/interface.h
> +++ b/interface.h
> @@ -82,4 +82,12 @@ void interface_set_name(struct interface *iface, const
> char *name);
> */
> bool interface_tsinfo_valid(struct interface *iface);
>
> +/**
> + * Tests whether an interface supports a set of given time stamping modes.
> + * @param iface The interface of interest.
> + * @param modes Bit mask of SOF_TIMESTAMPING_ flags.
> + * @return True if the time stamping modes are supported, false
> otherwise.
> + */
> +bool interface_tsmodes_supported(struct interface *iface, int modes);
> +
Good, the documentation comment indicates modes is a bitmask. In
otherwords, interface_tsmodes_supported can return true if modes is a
subset of the supported interface modes.
Ok.
> #endif
>
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH RFC 21/30] Convert call sites to the proper method for testing time stamp info validity.
On 2/11/2020 6:04 AM, Richard Cochran wrote:
> Signed-off-by: Richard Cochran
Straight forward.
Reviewed-by: Jacob Keller
> ---
> clock.c | 4 ++--
> port.c | 4 ++--
> 2 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/clock.c b/clock.c
> index 71b3899..845e27a 100644
> --- a/clock.c
> +++ b/clock.c
> @@ -956,7 +956,7 @@ struct clock *clock_create(enum clock_type type, struct
> config *config,
> interface_set_label(iface, ts_label);
> interface_ensure_tslabel(iface);
> interface_get_tsinfo(iface);
> - if (iface->ts_info.valid &&
> + if (interface_tsinfo_valid(iface) &&
> ((iface->ts_info.so_timestamping & required_modes) !=
> required_modes)) {
> pr_err("interface '%s' does not support requested
> timestamping mode",
> interface_name(iface));
> @@ -975,7 +975,7 @@ struct clock *clock_create(enum clock_type type, struct
> config *config,
> if (1 != sscanf(phc_device, "/dev/ptp%d", _index)) {
> phc_index = -1;
> }
> - } else if (iface->ts_info.valid) {
> + } else if (interface_tsinfo_valid(iface)) {
> phc_index = interface_phc_index(iface);
> } else {
> pr_err("PTP device not specified and automatic determination"
> diff --git a/port.c b/port.c
> index f4834ba..b590024 100644
> --- a/port.c
> +++ b/port.c
> @@ -2511,7 +2511,7 @@ void port_link_status(void *ctx, int linkup, int
> ts_index)
> interface_get_tsinfo(p->iface);
>
> /* Only switch phc with HW time stamping mode */
> - if (p->iface->ts_info.valid &&
> + if (interface_tsinfo_valid(p->iface) &&
> interface_phc_index(p->iface) >= 0) {
> required_modes = clock_required_modes(p->clock);
> if ((p->iface->ts_info.so_timestamping &
> required_modes) != required_modes) {
> @@ -3001,7 +3001,7 @@ struct port *port_open(const char *phc_device,
>
> if (transport == TRANS_UDS) {
> ; /* UDS cannot have a PHC. */
> - } else if (!interface->ts_info.valid) {
> + } else if (!interface_tsinfo_valid(interface)) {
> pr_warning("port %d: get_ts_info not supported", number);
> } else if (phc_index >= 0 &&
> phc_index != interface_phc_index(interface)) {
>
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH RFC 20/30] interface: Introduce a method to test the time stamping information validity.
On 2/11/2020 6:04 AM, Richard Cochran wrote:
> Signed-off-by: Richard Cochran
Reviewed-by: Jacob Keller
> ---
> interface.c | 5 +
> interface.h | 8
> 2 files changed, 13 insertions(+)
>
> diff --git a/interface.c b/interface.c
> index 02f63a0..7a3eddc 100644
> --- a/interface.c
> +++ b/interface.c
> @@ -42,3 +42,8 @@ void interface_set_name(struct interface *iface, const char
> *name)
> {
> strncpy(iface->name, name, MAX_IFNAME_SIZE);
> }
> +
> +bool interface_tsinfo_valid(struct interface *iface)
> +{
> + return iface->ts_info.valid ? true : false;
> +}
Do you actually need the ternary here? shouldn't ts_info.valid get
converted to true or false because we are returning a boolean?
I don't think this is harmful and you may consider it improving
readability though.
Thanks,
Jake
> diff --git a/interface.h b/interface.h
> index 4f408d5..3526a48 100644
> --- a/interface.h
> +++ b/interface.h
> @@ -7,6 +7,7 @@
> #ifndef HAVE_INTERFACE_H
> #define HAVE_INTERFACE_H
>
> +#include
> #include
> #include "sk.h"
>
> @@ -74,4 +75,11 @@ void interface_set_label(struct interface *iface, const
> char *label);
> */
> void interface_set_name(struct interface *iface, const char *name);
>
> +/**
> + * Tests whether an interface's time stamping information is valid or not.
> + * @param iface The interface of interest.
> + * @return True if the time stamping information is valid, false
> otherwise.
> + */
> +bool interface_tsinfo_valid(struct interface *iface);
> +
> #endif
>
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH RFC 18/30] interface: Introduce a method to get the PHC index.
On 2/11/2020 6:04 AM, Richard Cochran wrote:
> Signed-off-by: Richard Cochran
> ---
> interface.c | 5 +
> interface.h | 7 +++
> 2 files changed, 12 insertions(+)
>
> diff --git a/interface.c b/interface.c
> index d7eeb41..02f63a0 100644
> --- a/interface.c
> +++ b/interface.c
> @@ -28,6 +28,11 @@ const char *interface_name(struct interface *iface)
> return iface->name;
> }
>
> +int interface_phc_index(struct interface *iface)
> +{
> + return iface->ts_info.phc_index;
> +}
In theory these "getters" could use const struct interface *iface. I'm
not really sure that buys us much overall, though.
Thanks,
Jake
> +
> void interface_set_label(struct interface *iface, const char *label)
> {
> strncpy(iface->ts_label, label, MAX_IFNAME_SIZE);
> diff --git a/interface.h b/interface.h
> index f416b24..4f408d5 100644
> --- a/interface.h
> +++ b/interface.h
> @@ -53,6 +53,13 @@ const char *interface_label(struct interface *iface);
> */
> const char *interface_name(struct interface *iface);
>
> +/**
> + * Obtains the index of a PTP Hardware Clock device from a network interface.
> + * @param iface The interface of interest.
> + * @return The PHC index of the interface.
> + */
> +int interface_phc_index(struct interface *iface);
> +
> /**
> * Set the time stamping label of a given interface.
> * @param iface The interface of interest.
>
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH RFC 19/30] Convert call sites to the proper method for getting the PHC index.
On 2/11/2020 6:04 AM, Richard Cochran wrote:
> Signed-off-by: Richard Cochran
Reviewed-by: Jacob Keller
> ---
> clock.c | 2 +-
> port.c | 17 ++---
> 2 files changed, 11 insertions(+), 8 deletions(-)
>
> diff --git a/clock.c b/clock.c
> index 5001e66..71b3899 100644
> --- a/clock.c
> +++ b/clock.c
> @@ -976,7 +976,7 @@ struct clock *clock_create(enum clock_type type, struct
> config *config,
> phc_index = -1;
> }
> } else if (iface->ts_info.valid) {
> - phc_index = iface->ts_info.phc_index;
> + phc_index = interface_phc_index(iface);
> } else {
> pr_err("PTP device not specified and automatic determination"
> " is not supported. Please specify PTP device.");
> diff --git a/port.c b/port.c
> index c20c3fc..f4834ba 100644
> --- a/port.c
> +++ b/port.c
> @@ -2511,15 +2511,16 @@ void port_link_status(void *ctx, int linkup, int
> ts_index)
> interface_get_tsinfo(p->iface);
>
> /* Only switch phc with HW time stamping mode */
> - if (p->iface->ts_info.valid && p->iface->ts_info.phc_index >=
> 0) {
> + if (p->iface->ts_info.valid &&
> + interface_phc_index(p->iface) >= 0) {
> required_modes = clock_required_modes(p->clock);
> if ((p->iface->ts_info.so_timestamping &
> required_modes) != required_modes) {
> pr_err("interface '%s' does not support
> requested "
> "timestamping mode, set link status down
> by force.",
> interface_label(p->iface));
> p->link_status = LINK_DOWN | LINK_STATE_CHANGED;
> - } else if (p->phc_index != p->iface->ts_info.phc_index)
> {
> - p->phc_index = p->iface->ts_info.phc_index;
> + } else if (p->phc_index !=
> interface_phc_index(p->iface)) {
> + p->phc_index = interface_phc_index(p->iface);
>
> if (clock_switch_phc(p->clock, p->phc_index)) {
> p->last_fault_type = FT_SWITCH_PHC;
> @@ -3002,19 +3003,21 @@ struct port *port_open(const char *phc_device,
> ; /* UDS cannot have a PHC. */
> } else if (!interface->ts_info.valid) {
> pr_warning("port %d: get_ts_info not supported", number);
> - } else if (phc_index >= 0 && phc_index != interface->ts_info.phc_index)
> {
> + } else if (phc_index >= 0 &&
> +phc_index != interface_phc_index(interface)) {
> if (p->jbod) {
> pr_warning("port %d: just a bunch of devices", number);
> - p->phc_index = interface->ts_info.phc_index;
> + p->phc_index = interface_phc_index(interface);
> } else if (phc_device) {
> pr_warning("port %d: taking %s from the command line, "
> "not the attached ptp%d", number, phc_device,
> -interface->ts_info.phc_index);
> +interface_phc_index(interface));
> p->phc_index = phc_index;
> } else {
> pr_err("port %d: PHC device mismatch", number);
> pr_err("port %d: /dev/ptp%d requested, ptp%d attached",
> -number, phc_index, interface->ts_info.phc_index);
> +number, phc_index,
> +interface_phc_index(interface));
> goto err_port;
> }
> }
>
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH RFC 18/30] interface: Introduce a method to get the PHC index.
On 2/11/2020 6:04 AM, Richard Cochran wrote:
> Signed-off-by: Richard Cochran
Reviewed-by: Jacob Keller
> ---
> interface.c | 5 +
> interface.h | 7 +++
> 2 files changed, 12 insertions(+)
>
> diff --git a/interface.c b/interface.c
> index d7eeb41..02f63a0 100644
> --- a/interface.c
> +++ b/interface.c
> @@ -28,6 +28,11 @@ const char *interface_name(struct interface *iface)
> return iface->name;
> }
>
> +int interface_phc_index(struct interface *iface)
> +{
> + return iface->ts_info.phc_index;
> +}
> +
> void interface_set_label(struct interface *iface, const char *label)
> {
> strncpy(iface->ts_label, label, MAX_IFNAME_SIZE);
> diff --git a/interface.h b/interface.h
> index f416b24..4f408d5 100644
> --- a/interface.h
> +++ b/interface.h
> @@ -53,6 +53,13 @@ const char *interface_label(struct interface *iface);
> */
> const char *interface_name(struct interface *iface);
>
> +/**
> + * Obtains the index of a PTP Hardware Clock device from a network interface.
> + * @param iface The interface of interest.
> + * @return The PHC index of the interface.
> + */
> +int interface_phc_index(struct interface *iface);
> +
> /**
> * Set the time stamping label of a given interface.
> * @param iface The interface of interest.
>
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH RFC 17/30] Convert call sites to the proper method for setting the time stamping label.
On 2/11/2020 6:04 AM, Richard Cochran wrote:
> Signed-off-by: Richard Cochran > ---
> clock.c | 6 --
> nsm.c | 5 -
> port.c | 2 +-
> rtnl.c | 2 +-
> rtnl.h | 4 +++-
> 5 files changed, 13 insertions(+), 6 deletions(-)
>
> diff --git a/clock.c b/clock.c
> index 159fcb2..5001e66 100644
> --- a/clock.c
> +++ b/clock.c
> @@ -846,6 +846,7 @@ struct clock *clock_create(enum clock_type type, struct
> config *config,
> const char *phc_device)
> {
> enum servo_type servo = config_get_int(config, NULL, "clock_servo");
> + char ts_label[IF_NAMESIZE], phc[32], *tmp;
> enum timestamp_type timestamping;
> int fadj = 0, max_adj = 0, sw_ts;
> int phc_index, required_modes = 0;
> @@ -853,7 +854,6 @@ struct clock *clock_create(enum clock_type type, struct
> config *config,
> const char *uds_ifname;
> struct port *p;
> unsigned char oui[OUI_LEN];
> - char phc[32], *tmp;
> struct interface *iface, *udsif = >uds_interface;
> struct timespec ts;
> int sfl;
> @@ -951,7 +951,9 @@ struct clock *clock_create(enum clock_type type, struct
> config *config,
> c->timestamping = timestamping;
> required_modes = clock_required_modes(c);
> STAILQ_FOREACH(iface, >interfaces, list) {
> - rtnl_get_ts_device(interface_name(iface), iface->ts_label);
> + memset(ts_label, 0, sizeof(ts_label));
> + rtnl_get_ts_device(interface_name(iface), ts_label);
> + interface_set_label(iface, ts_label);
Hmm.. odd that we now add a memset vs the previous that didn't use one?
What was the reasoning..? In either case unless rtnl_get_ts_device
doesn't null terminate its output, we should be safe in using strncpy
since it would stop at the NULL terminator or the maximum size.
Thanks,
Jake
> interface_ensure_tslabel(iface);
> interface_get_tsinfo(iface);
> if (iface->ts_info.valid &&
> diff --git a/nsm.c b/nsm.c
> index e82fc37..1292c6b 100644
> --- a/nsm.c
> +++ b/nsm.c
> @@ -262,13 +262,16 @@ static void nsm_help(FILE *fp)
> static int nsm_open(struct nsm *nsm, struct config *cfg)
> {
> enum transport_type transport;
> + char ts_label[IF_NAMESIZE];
> const char *ifname, *name;
> struct interface *iface;
> int count = 0;
>
> STAILQ_FOREACH(iface, >interfaces, list) {
> ifname = interface_name(iface);
> - rtnl_get_ts_device(ifname, iface->ts_label);
> + memset(ts_label, 0, sizeof(ts_label));
> + rtnl_get_ts_device(ifname, ts_label);
> + interface_set_label(iface, ts_label);
> interface_ensure_tslabel(iface);
> count++;
> }
> diff --git a/port.c b/port.c
> index 05eb1d6..c20c3fc 100644
> --- a/port.c
> +++ b/port.c
> @@ -2500,7 +2500,7 @@ void port_link_status(void *ctx, int linkup, int
> ts_index)
> /* ts_label changed */
> old_ts_label = interface_label(p->iface);
> if (if_indextoname(ts_index, ts_label) && strcmp(old_ts_label,
> ts_label)) {
> - strncpy(p->iface->ts_label, ts_label, MAX_IFNAME_SIZE);
> + interface_set_label(p->iface, ts_label);
> p->link_status |= TS_LABEL_CHANGED;
> pr_notice("port %hu: ts label changed to %s", portnum(p),
> ts_label);
> }
> diff --git a/rtnl.c b/rtnl.c
> index d9c76d7..b7a2667 100644
> --- a/rtnl.c
> +++ b/rtnl.c
> @@ -87,7 +87,7 @@ static void rtnl_get_ts_device_callback(void *ctx, int
> linkup, int ts_index)
> *dst = ts_index;
> }
>
> -int rtnl_get_ts_device(const char *device, char *ts_device)
> +int rtnl_get_ts_device(const char *device, char ts_device[IF_NAMESIZE])
> {
> int err, fd;
> int ts_index = -1;
> diff --git a/rtnl.h b/rtnl.h
> index c5ea979..8fef4a9 100644
> --- a/rtnl.h
> +++ b/rtnl.h
> @@ -20,6 +20,8 @@
> #ifndef HAVE_RTNL_H
> #define HAVE_RTNL_H
>
> +#include
> +
> typedef void (*rtnl_callback)(void *ctx, int linkup, int ts_index);
>
> /**
> @@ -37,7 +39,7 @@ int rtnl_close(int fd);
> * at least IF_NAMESIZE bytes long.
> * @return Zero on success, or -1 on error.
> */
> -int rtnl_get_ts_device(const char *device, char *ts_device);
> +int rtnl_get_ts_device(const char *device, char ts_device[IF_NAMESIZE]);
>
> /**
> * Request the link status from the kernel.
>
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH RFC 16/30] interface: Introduce a method to set the time stamping label.
On 2/11/2020 6:04 AM, Richard Cochran wrote:
> The ts_label field of the interface is set in different ways by different
> callers. In order to prevent users from open coding the logic that sets
> the label, this patch adds an appropriate method.
>
> Signed-off-by: Richard Cochran
Reviewed-by: Jacob Keller
> ---
> interface.c | 5 +
> interface.h | 7 +++
> 2 files changed, 12 insertions(+)
>
> diff --git a/interface.c b/interface.c
> index 3811679..d7eeb41 100644
> --- a/interface.c
> +++ b/interface.c
> @@ -28,6 +28,11 @@ const char *interface_name(struct interface *iface)
> return iface->name;
> }
>
> +void interface_set_label(struct interface *iface, const char *label)
> +{
> + strncpy(iface->ts_label, label, MAX_IFNAME_SIZE);
> +}
Same here, it might be a good idea to ensure that last byte
(MAX_IFNAME_SIZE + 1) is set to '\0'.
> +
> void interface_set_name(struct interface *iface, const char *name)
> {
> strncpy(iface->name, name, MAX_IFNAME_SIZE);
> diff --git a/interface.h b/interface.h
> index 5f449ae..f416b24 100644
> --- a/interface.h
> +++ b/interface.h
> @@ -53,6 +53,13 @@ const char *interface_label(struct interface *iface);
> */
> const char *interface_name(struct interface *iface);
>
> +/**
> + * Set the time stamping label of a given interface.
> + * @param iface The interface of interest.
> + * @param name The desired label for the interface.
> + */
> +void interface_set_label(struct interface *iface, const char *label);
> +
> /**
> * Set the name of a given interface.
> * @param iface The interface of interest.
>
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH RFC 15/30] Convert call sites to the proper method for setting the name.
On 2/11/2020 6:04 AM, Richard Cochran wrote:
> Signed-off-by: Richard Cochran
Reviewed-by: Jacob Keller
> ---
> clock.c | 5 +++--
> config.c | 2 +-
> pmc_common.c | 2 +-
> 3 files changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/clock.c b/clock.c
> index 3895d09..159fcb2 100644
> --- a/clock.c
> +++ b/clock.c
> @@ -850,6 +850,7 @@ struct clock *clock_create(enum clock_type type, struct
> config *config,
> int fadj = 0, max_adj = 0, sw_ts;
> int phc_index, required_modes = 0;
> struct clock *c = _clock;
> + const char *uds_ifname;
> struct port *p;
> unsigned char oui[OUI_LEN];
> char phc[32], *tmp;
> @@ -999,8 +1000,8 @@ struct clock *clock_create(enum clock_type type, struct
> config *config,
> }
>
> /* Configure the UDS. */
> - snprintf(udsif->name, sizeof(udsif->name), "%s",
> - config_get_string(config, NULL, "uds_address"));
> + uds_ifname = config_get_string(config, NULL, "uds_address");
> + interface_set_name(udsif, uds_ifname);
This used to do an snprintf and now it does a strncpy. This is slightly
different because snprintf will null terminate while strncpy won't
guarantee this.. However we now copy by MAX_IFNAME_SIZE rather than
snprintf's "full" size. This means that the function will guarantee to
be null terminated if the original data starts as zero-allocated.
Perhaps the interface_set_name should guarantee the last byte is zero
just in case the interface structure is not per-initialized with zeros.
Thanks,
Jake
> if (config_set_section_int(config, interface_name(udsif),
> "announceReceiptTimeout", 0)) {
> return NULL;
> diff --git a/config.c b/config.c
> index c30f6bc..717ee65 100644
> --- a/config.c
> +++ b/config.c
> @@ -835,7 +835,7 @@ struct interface *config_create_interface(const char
> *name, struct config *cfg)
> return NULL;
> }
>
> - strncpy(iface->name, name, MAX_IFNAME_SIZE);
> + interface_set_name(iface, name);
> STAILQ_INSERT_TAIL(>interfaces, iface, list);
> cfg->n_interfaces++;
>
> diff --git a/pmc_common.c b/pmc_common.c
> index 6bdaa94..41181fb 100644
> --- a/pmc_common.c
> +++ b/pmc_common.c
> @@ -350,7 +350,7 @@ struct pmc *pmc_create(struct config *cfg, enum
> transport_type transport_type,
> goto failed;
> }
>
> - strncpy(iface.name, iface_name, MAX_IFNAME_SIZE);
> + interface_set_name(, iface_name);
> interface_ensure_tslabel();
>
> if (transport_open(pmc->transport, ,
>
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH RFC 13/30] Convert call sites to the proper method for initializing the time stamping label.
On 2/11/2020 6:04 AM, Richard Cochran wrote:
> Signed-off-by: Richard Cochran
> ---
> clock.c | 12 +---
> nsm.c| 4 +---
> pmc_common.c | 4 +---
> 3 files changed, 3 insertions(+), 17 deletions(-)
>
Reviewed-by: Jacob Keller
> diff --git a/clock.c b/clock.c
> index f987965..3895d09 100644
> --- a/clock.c
> +++ b/clock.c
> @@ -842,16 +842,6 @@ int clock_required_modes(struct clock *c)
> return required_modes;
> }
>
> -/*
> - * If we do not have a slave or the rtnl query failed, then use our
> - * own interface name as the time stamping interface name.
> - */
> -static void ensure_ts_label(struct interface *iface)
> -{
> - if (iface->ts_label[0] == '\0')
> - strncpy(iface->ts_label, interface_name(iface),
> MAX_IFNAME_SIZE);
> -}
> -
Removing both open-coded and the implementation in another .c file. Nice!
Thanks,
Jake
> struct clock *clock_create(enum clock_type type, struct config *config,
> const char *phc_device)
> {
> @@ -961,7 +951,7 @@ struct clock *clock_create(enum clock_type type, struct
> config *config,
> required_modes = clock_required_modes(c);
> STAILQ_FOREACH(iface, >interfaces, list) {
> rtnl_get_ts_device(interface_name(iface), iface->ts_label);
> - ensure_ts_label(iface);
> + interface_ensure_tslabel(iface);
> interface_get_tsinfo(iface);
> if (iface->ts_info.valid &&
> ((iface->ts_info.so_timestamping & required_modes) !=
> required_modes)) {
> diff --git a/nsm.c b/nsm.c
> index 269c3c8..e82fc37 100644
> --- a/nsm.c
> +++ b/nsm.c
> @@ -269,9 +269,7 @@ static int nsm_open(struct nsm *nsm, struct config *cfg)
> STAILQ_FOREACH(iface, >interfaces, list) {
> ifname = interface_name(iface);
> rtnl_get_ts_device(ifname, iface->ts_label);
> - if (iface->ts_label[0] == '\0') {
> - strncpy(iface->ts_label, ifname, MAX_IFNAME_SIZE);
> - }
> + interface_ensure_tslabel(iface);
> count++;
> }
> if (count != 1) {
> diff --git a/pmc_common.c b/pmc_common.c
> index d5c8b61..6bdaa94 100644
> --- a/pmc_common.c
> +++ b/pmc_common.c
> @@ -351,9 +351,7 @@ struct pmc *pmc_create(struct config *cfg, enum
> transport_type transport_type,
> }
>
> strncpy(iface.name, iface_name, MAX_IFNAME_SIZE);
> - if (iface.ts_label[0] == '\0') {
> - strncpy(iface.ts_label, interface_name(),
> MAX_IFNAME_SIZE);
> - }
> + interface_ensure_tslabel();
>
> if (transport_open(pmc->transport, ,
> >fdarray, TS_SOFTWARE)) {
>
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH RFC 14/30] interface: Introduce a method to set the name.
On 2/11/2020 6:04 AM, Richard Cochran wrote:
> The name field of the interface is set in different ways by different
> callers. In order to prevent users from open coding the logic that sets
> the name, this patch adds an appropriate method.
>
> Signed-off-by: Richard Cochran
Reviewed-by: Jacob Keller
> ---
> interface.c | 5 +
> interface.h | 8 +++-
> 2 files changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/interface.c b/interface.c
> index 662552d..3811679 100644
> --- a/interface.c
> +++ b/interface.c
> @@ -27,3 +27,8 @@ const char *interface_name(struct interface *iface)
> {
> return iface->name;
> }
> +
> +void interface_set_name(struct interface *iface, const char *name)
> +{
> + strncpy(iface->name, name, MAX_IFNAME_SIZE);
> +}
Good, the name is marked as constant. Side note, for those interface_*
functions that don't modify the interface, does it make sense to mark
them as taking a const interface pointer?
> diff --git a/interface.h b/interface.h
> index 371185d..5f449ae 100644
> --- a/interface.h
> +++ b/interface.h
> @@ -53,5 +53,11 @@ const char *interface_label(struct interface *iface);
> */
> const char *interface_name(struct interface *iface);
>
> -#endif
> +/**
> + * Set the name of a given interface.
> + * @param iface The interface of interest.
> + * @param name The desired name for the interface.
> + */
> +void interface_set_name(struct interface *iface, const char *name);
>
> +#endif
>
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH RFC 12/30] interface: Introduce a method to initialize the time stamping label.
On 2/11/2020 6:04 AM, Richard Cochran wrote:
> In many cases, the time stamping label will be the same as the name of
> the interface. In order to prevent users from open coding the logic that
> initializes the label from the interface name, this patch add an
> appropriate method.
>
> Signed-off-by: Richard Cochran
> ---
> interface.c | 7 +++
> interface.h | 6 ++
> 2 files changed, 13 insertions(+)
>
> diff --git a/interface.c b/interface.c
> index 460ceb8..662552d 100644
> --- a/interface.c
> +++ b/interface.c
> @@ -6,6 +6,13 @@
> */
> #include "interface.h"
>
> +void interface_ensure_tslabel(struct interface *iface)
> +{
> + if (!iface->ts_label[0]) {
The original code did "if (label[0] == '\0')" but this is equivalent. Ok.
> + strncpy(iface->ts_label, iface->name, MAX_IFNAME_SIZE);
> + }
> +}
> +
> int interface_get_tsinfo(struct interface *iface)
> {
> return sk_get_ts_info(iface->ts_label, >ts_info);
> diff --git a/interface.h b/interface.h
> index 05cfb10..371185d 100644
> --- a/interface.h
> +++ b/interface.h
> @@ -24,6 +24,12 @@ struct interface {
> struct sk_ts_info ts_info;
> };
>
> +/**
> + * Ensures that an interface has a proper time stamping label.
> + * @param iface The interface of interest.
> + */
> +void interface_ensure_tslabel(struct interface *iface);
> +
> /**
> * Populate the time stamping information of a given interface.
> * @param iface The interface of interest.
>
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH RFC 12/30] interface: Introduce a method to initialize the time stamping label.
On 2/11/2020 6:04 AM, Richard Cochran wrote:
> In many cases, the time stamping label will be the same as the name of
> the interface. In order to prevent users from open coding the logic that
> initializes the label from the interface name, this patch add an
> appropriate method.
>
> Signed-off-by: Richard Cochran
Reviewed-by: Jacob Keller
> ---
> interface.c | 7 +++
> interface.h | 6 ++
> 2 files changed, 13 insertions(+)
>
> diff --git a/interface.c b/interface.c
> index 460ceb8..662552d 100644
> --- a/interface.c
> +++ b/interface.c
> @@ -6,6 +6,13 @@
> */
> #include "interface.h"
>
> +void interface_ensure_tslabel(struct interface *iface)
> +{
> + if (!iface->ts_label[0]) {
> + strncpy(iface->ts_label, iface->name, MAX_IFNAME_SIZE);
> + }
> +}
> +
> int interface_get_tsinfo(struct interface *iface)
> {
> return sk_get_ts_info(iface->ts_label, >ts_info);
> diff --git a/interface.h b/interface.h
> index 05cfb10..371185d 100644
> --- a/interface.h
> +++ b/interface.h
> @@ -24,6 +24,12 @@ struct interface {
> struct sk_ts_info ts_info;
> };
>
> +/**
> + * Ensures that an interface has a proper time stamping label.
> + * @param iface The interface of interest.
> + */
> +void interface_ensure_tslabel(struct interface *iface);
> +
> /**
> * Populate the time stamping information of a given interface.
> * @param iface The interface of interest.
>
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH RFC 11/30] Convert call sites to the proper method for getting time stamp information.
On 2/11/2020 6:04 AM, Richard Cochran wrote:
> Signed-off-by: Richard Cochran
Reviewed-by: Jacob Keller
> ---
> clock.c | 2 +-
> port.c | 2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/clock.c b/clock.c
> index 66c6bc1..f987965 100644
> --- a/clock.c
> +++ b/clock.c
> @@ -962,7 +962,7 @@ struct clock *clock_create(enum clock_type type, struct
> config *config,
> STAILQ_FOREACH(iface, >interfaces, list) {
> rtnl_get_ts_device(interface_name(iface), iface->ts_label);
> ensure_ts_label(iface);
Unrelated to this patch, but I imagine this function wants to be moved
to interface.o too?
> - sk_get_ts_info(interface_label(iface), >ts_info);
> + interface_get_tsinfo(iface);
> if (iface->ts_info.valid &&
> ((iface->ts_info.so_timestamping & required_modes) !=
> required_modes)) {
> pr_err("interface '%s' does not support requested
> timestamping mode",
> diff --git a/port.c b/port.c
> index 52aef86..05eb1d6 100644
> --- a/port.c
> +++ b/port.c
> @@ -2508,7 +2508,7 @@ void port_link_status(void *ctx, int linkup, int
> ts_index)
> /* Both link down/up and change ts_label may change phc index. */
> if (p->link_status & LINK_UP &&
> (p->link_status & LINK_STATE_CHANGED || p->link_status &
> TS_LABEL_CHANGED)) {
> - sk_get_ts_info(interface_label(p->iface), >iface->ts_info);
> + interface_get_tsinfo(p->iface);
>
> /* Only switch phc with HW time stamping mode */
> if (p->iface->ts_info.valid && p->iface->ts_info.phc_index >=
> 0) {
>
Hmm. So we will also need an accessor for these pieces of data, but that
looks like it's handled in a future patch.
Thanks,
Jake
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH RFC 10/30] interface: Introduce a method to get the time stamping information.
On 2/11/2020 6:04 AM, Richard Cochran wrote:
> In order to prevent users from open coding this logic, this patch
> provides a method that populates the time stamping information from
> the interface label.
>
> Signed-off-by: Richard Cochran
Makes sense.
Reviewed-by: Jacob Keller
> ---
> interface.c | 5 +
> interface.h | 7 +++
> 2 files changed, 12 insertions(+)
>
> diff --git a/interface.c b/interface.c
> index 7909a5e..460ceb8 100644
> --- a/interface.c
> +++ b/interface.c
> @@ -6,6 +6,11 @@
> */
> #include "interface.h"
>
> +int interface_get_tsinfo(struct interface *iface)
> +{
> + return sk_get_ts_info(iface->ts_label, >ts_info);
> +}
> +
Presumably callers don't need to directly access ts_info, or if they do
we can later provide an accessor function.
> const char *interface_label(struct interface *iface)
> {
> return iface->ts_label;
> diff --git a/interface.h b/interface.h
> index 89f3e94..05cfb10 100644
> --- a/interface.h
> +++ b/interface.h
> @@ -24,6 +24,13 @@ struct interface {
> struct sk_ts_info ts_info;
> };
>
> +/**
> + * Populate the time stamping information of a given interface.
> + * @param iface The interface of interest.
> + * @return zero on success, negative on failure.
> + */
> +int interface_get_tsinfo(struct interface *iface);
> +
> /**
> * Obtain the time stamping label of a network interface. This can be
> * different from the name of the interface when bonding is in effect.
>
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH RFC 09/30] Convert call sites to the proper method for getting interface labels.
On 2/11/2020 6:03 AM, Richard Cochran wrote:
> Signed-off-by: Richard Cochran
Straight forward.
Besides wondering about the object groups in the makefile,
Reviewed-by: Jacob Keller
> ---
> clock.c | 2 +-
> port.c | 17 ++---
> raw.c | 5 +++--
> udp.c | 2 +-
> udp6.c | 2 +-
> 5 files changed, 16 insertions(+), 12 deletions(-)
>
> diff --git a/clock.c b/clock.c
> index 7d13b3b..66c6bc1 100644
> --- a/clock.c
> +++ b/clock.c
> @@ -962,7 +962,7 @@ struct clock *clock_create(enum clock_type type, struct
> config *config,
> STAILQ_FOREACH(iface, >interfaces, list) {
> rtnl_get_ts_device(interface_name(iface), iface->ts_label);
> ensure_ts_label(iface);
> - sk_get_ts_info(iface->ts_label, >ts_info);
> + sk_get_ts_info(interface_label(iface), >ts_info);
> if (iface->ts_info.valid &&
> ((iface->ts_info.so_timestamping & required_modes) !=
> required_modes)) {
> pr_err("interface '%s' does not support requested
> timestamping mode",
> diff --git a/port.c b/port.c
> index 6423568..52aef86 100644
> --- a/port.c
> +++ b/port.c
> @@ -792,6 +792,7 @@ static int port_management_fill_response(struct port
> *target,
> struct management_tlv *tlv;
> struct port_ds_np *pdsnp;
> struct tlv_extra *extra;
> + const char *ts_label;
> struct portDS *pds;
> uint16_t u16;
> uint8_t *buf;
> @@ -941,7 +942,8 @@ static int port_management_fill_response(struct port
> *target,
> else
> ppn->port_state = target->state;
> ppn->timestamping = target->timestamping;
> - ptp_text_set(>interface, target->iface->ts_label);
> + ts_label = interface_label(target->iface);
> + ptp_text_set(>interface, ts_label);
> datalen = sizeof(*ppn) + ppn->interface.length;
> break;
> case TLV_PORT_STATS_NP:
> @@ -2482,10 +2484,10 @@ static void bc_dispatch(struct port *p, enum
> fsm_event event, int mdiff)
>
> void port_link_status(void *ctx, int linkup, int ts_index)
> {
> - struct port *p = ctx;
> - int link_state;
> char ts_label[MAX_IFNAME_SIZE + 1] = {0};
> - int required_modes;
> + int link_state, required_modes;
> + const char *old_ts_label;
> + struct port *p = ctx;
>
> link_state = linkup ? LINK_UP : LINK_DOWN;
> if (p->link_status & link_state) {
> @@ -2496,7 +2498,8 @@ void port_link_status(void *ctx, int linkup, int
> ts_index)
> }
>
> /* ts_label changed */
> - if (if_indextoname(ts_index, ts_label) && strcmp(p->iface->ts_label,
> ts_label)) {
> + old_ts_label = interface_label(p->iface);
> + if (if_indextoname(ts_index, ts_label) && strcmp(old_ts_label,
> ts_label)) {
> strncpy(p->iface->ts_label, ts_label, MAX_IFNAME_SIZE);
> p->link_status |= TS_LABEL_CHANGED;
> pr_notice("port %hu: ts label changed to %s", portnum(p),
> ts_label);
> @@ -2505,7 +2508,7 @@ void port_link_status(void *ctx, int linkup, int
> ts_index)
> /* Both link down/up and change ts_label may change phc index. */
> if (p->link_status & LINK_UP &&
> (p->link_status & LINK_STATE_CHANGED || p->link_status &
> TS_LABEL_CHANGED)) {
> - sk_get_ts_info(p->iface->ts_label, >iface->ts_info);
> + sk_get_ts_info(interface_label(p->iface), >iface->ts_info);
>
> /* Only switch phc with HW time stamping mode */
> if (p->iface->ts_info.valid && p->iface->ts_info.phc_index >=
> 0) {
> @@ -2513,7 +2516,7 @@ void port_link_status(void *ctx, int linkup, int
> ts_index)
> if ((p->iface->ts_info.so_timestamping &
> required_modes) != required_modes) {
> pr_err("interface '%s' does not support
> requested "
> "timestamping mode, set link status down
> by force.",
> -p->iface->ts_label);
> +interface_label(p->iface));
> p->link_status = LINK_DOWN | LINK_STATE_CHANGED;
> } else if (p->phc_index != p->iface->ts_info.phc_index)
> {
> p->phc_index = p->iface->ts_info.phc_index;
> diff --git a/raw.c b/raw.c
> index f1c92b9..81ec431 100644
> --- a/raw.c
> +++ b/raw.c
> @@ -213,9 +213,10 @@ static int raw_open(struct transport *t, struct
> interface *iface,
> unsigned char ptp_dst_mac[MAC_LEN];
> unsigned char p2p_dst_mac[MAC_LEN];
> int efd, gfd, socket_priority;
> - char *str, *name;
> + const char *name;
> + char *str;
>
> - name = iface->ts_label;
> + name = interface_label(iface);
> str = config_get_string(t->cfg, name, "ptp_dst_mac");
> if (str2mac(str, ptp_dst_mac)) {
> pr_err("invalid
Re: [Linuxptp-devel] [PATCH RFC 08/30] interface: Introduce an access method for the time stamping label.
On 2/11/2020 6:03 AM, Richard Cochran wrote:
> Many of the users only require a read only reference to the time
> stamping label of the interface. This patch adds an appropriate
> method.
>
> Signed-off-by: Richard Cochran
Makes sense.
> ---
> interface.c | 5 +
> interface.h | 9 +
> 2 files changed, 14 insertions(+)
>
> diff --git a/interface.c b/interface.c
> index 1231db9..7909a5e 100644
> --- a/interface.c
> +++ b/interface.c
> @@ -6,6 +6,11 @@
> */
> #include "interface.h"
>
> +const char *interface_label(struct interface *iface)
> +{
> + return iface->ts_label;
> +}
> +
> const char *interface_name(struct interface *iface)
> {
> return iface->name;
> diff --git a/interface.h b/interface.h
> index 94d5b8f..89f3e94 100644
> --- a/interface.h
> +++ b/interface.h
> @@ -24,6 +24,15 @@ struct interface {
> struct sk_ts_info ts_info;
> };
>
> +/**
> + * Obtain the time stamping label of a network interface. This can be
> + * different from the name of the interface when bonding is in effect.
> + *
> + * @param iface The interface of interest.
> + * @return The time stamping device name of the network interface.
> + */
> +const char *interface_label(struct interface *iface);
> +
Nice to see this documented, and explaining why it might be different.
Thanks,
Jake
> /**
> * Obtains the name of a network interface.
> * @param iface The interface of interest.
>
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH RFC 08/30] interface: Introduce an access method for the time stamping label.
On 2/11/2020 6:03 AM, Richard Cochran wrote: > Many of the users only require a read only reference to the time > stamping label of the interface. This patch adds an appropriate > method. > > Signed-off-by: Richard Cochran Reviewed-by: Jacob Keller ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH RFC 07/30] Convert call sites to the proper method for getting interface names.
On 2/11/2020 6:03 AM, Richard Cochran wrote:
> Signed-off-by: Richard Cochran
> ---
> clock.c| 21 +++--
> config.c | 12 +++-
> makefile | 10 +-
> nsm.c | 9 +
> pmc_common.c | 2 +-
> port.c | 19 +++
> port_private.h | 2 +-
> udp.c | 2 +-
> udp6.c | 2 +-
> uds.c | 8
> 10 files changed, 47 insertions(+), 40 deletions(-)
> diff --git a/config.c b/config.c
> index 65afa70..c30f6bc 100644
> --- a/config.c
> +++ b/config.c
> @@ -775,15 +775,15 @@ int config_read(const char *name, struct config *cfg)
> if (parse_setting_line(line, , )) {
> fprintf(stderr, "could not parse line %d in %s
> section\n",
> line_num, current_section == GLOBAL_SECTION ?
> - "global" : current_port->name);
> + "global" : interface_name(current_port));
> goto parse_error;
> }
>
> check_deprecated_options();
>
> parser_res = parse_item(cfg, 0, current_section ==
> GLOBAL_SECTION ?
> - NULL : current_port->name, option,
> value);
> -
> + NULL : interface_name(current_port),
> + option, value);
> switch (parser_res) {
> case PARSED_OK:
> break;
> @@ -791,7 +791,7 @@ int config_read(const char *name, struct config *cfg)
> fprintf(stderr, "unknown option %s at line %d in %s
> section\n",
> option, line_num,
> current_section == GLOBAL_SECTION ? "global" :
> - current_port->name);
> + interface_name(current_port));
> goto parse_error;
> case BAD_VALUE:
> fprintf(stderr, "%s is a bad value for option %s at
> line %d\n",
> @@ -820,10 +820,12 @@ parse_error:
> struct interface *config_create_interface(const char *name, struct config
> *cfg)
> {
> struct interface *iface;
> + const char *ifname;
>
> /* only create each interface once (by name) */
> STAILQ_FOREACH(iface, >interfaces, list) {
> - if (0 == strncmp(name, iface->name, MAX_IFNAME_SIZE))
> + ifname = interface_name(iface);
> + if (0 == strncmp(name, ifname, MAX_IFNAME_SIZE))
> return iface;
> }
>
We use the new interface_name() in config.c meaning that all users of
config.o must link to interface.o now...
> diff --git a/makefile b/makefile
> index e1e0e99..e1dd3fa 100644
> --- a/makefile
> +++ b/makefile
> @@ -57,13 +57,13 @@ all: $(PRG)
>
> ptp4l: $(OBJ)
>
> -nsm: config.o $(FILTERS) hash.o msg.o nsm.o phc.o print.o \
> +nsm: config.o $(FILTERS) hash.o interface.o msg.o nsm.o phc.o print.o \
> rtnl.o sk.o $(TRANSP) tlv.o tsproc.o util.o version.o
>
> -pmc: config.o hash.o msg.o phc.o pmc.o pmc_common.o print.o sk.o tlv.o \
> - $(TRANSP) util.o version.o
> +pmc: config.o hash.o interface.o msg.o phc.o pmc.o pmc_common.o print.o sk.o
> \
> + tlv.o $(TRANSP) util.o version.o
>
> -phc2sys: clockadj.o clockcheck.o config.o hash.o msg.o \
> +phc2sys: clockadj.o clockcheck.o config.o hash.o interface.o msg.o \
> phc.o phc2sys.o pmc_common.o print.o $(SERVOS) sk.o stats.o \
> sysoff.o tlv.o $(TRANSP) util.o version.o
>
> @@ -71,7 +71,7 @@ hwstamp_ctl: hwstamp_ctl.o version.o
>
> phc_ctl: phc_ctl.o phc.o sk.o util.o clockadj.o sysoff.o print.o version.o
>
> -snmp4lptp: config.o hash.o msg.o phc.o pmc_common.o print.o sk.o \
> +snmp4lptp: config.o hash.o interface.o msg.o phc.o pmc_common.o print.o sk.o
> \
> snmp4lptp.o tlv.o $(TRANSP) util.o
> $(CC) $^ $(LDFLAGS) $(LOADLIBES) $(LDLIBS) $(snmplib) -o $@
>
Considering the interface logic used to be in config.h and all of the
modified programs load from config does it make sense to add $(CONFIG)
that selects both config.o and interface.o? I mean config.o now requires
interface.o
Hmm. On the one hand it sort of doesn't make that much sense because
interface stuff is distinct from configs?
Is there a way to generate the network of how interconnected the various
object files are?
Thanks,
Jake
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH RFC 06/30] interface: Introduce an access method for the name field.
On 2/11/2020 6:03 AM, Richard Cochran wrote:
> Many of the users only require a read only reference to the interface name.
> This patch adds an appropriate method.
>
Right.
> Signed-off-by: Richard Cochran ---
> interface.c | 12
> interface.h | 7 +++
> makefile| 8
> 3 files changed, 23 insertions(+), 4 deletions(-)
> create mode 100644 interface.c
>
> diff --git a/interface.c b/interface.c
> new file mode 100644
> index 000..1231db9
> --- /dev/null
> +++ b/interface.c
> @@ -0,0 +1,12 @@
> +/**
> + * @file interface.c
> + * @brief Implements network interface data structures.
> + * @note Copyright (C) 2020 Richard Cochran
> + * @note SPDX-License-Identifier: GPL-2.0+
> + */
> +#include "interface.h"
> +
> +const char *interface_name(struct interface *iface)
> +{
> + return iface->name;
> +}
> diff --git a/interface.h b/interface.h
> index 61d53a2..94d5b8f 100644
> --- a/interface.h
> +++ b/interface.h
> @@ -24,5 +24,12 @@ struct interface {
> struct sk_ts_info ts_info;
> };
>
> +/**
> + * Obtains the name of a network interface.
> + * @param iface The interface of interest.
> + * @return The device name of the network interface.
> + */
> +const char *interface_name(struct interface *iface);
> +
> #endif
>
> diff --git a/makefile b/makefile
> index d58d13a..e1e0e99 100644
> --- a/makefile
> +++ b/makefile
> @@ -27,10 +27,10 @@ FILTERS = filter.o mave.o mmedian.o
> SERVOS = linreg.o ntpshm.o nullf.o pi.o servo.o
> TRANSP = raw.o transport.o udp.o udp6.o uds.o
> OBJ = bmc.o clock.o clockadj.o clockcheck.o config.o designated_fsm.o \
> - e2e_tc.o fault.o $(FILTERS) fsm.o hash.o msg.o phc.o port.o
> port_signaling.o \
> - pqueue.o print.o ptp4l.o p2p_tc.o rtnl.o $(SERVOS) sk.o stats.o tc.o \
> - $(TRANSP) telecom.o tlv.o tsproc.o unicast_client.o unicast_fsm.o \
> - unicast_service.o util.o version.o
> + e2e_tc.o fault.o $(FILTERS) fsm.o hash.o interface.o msg.o phc.o port.o \
> + port_signaling.o pqueue.o print.o ptp4l.o p2p_tc.o rtnl.o $(SERVOS) sk.o \
> + stats.o tc.o $(TRANSP) telecom.o tlv.o tsproc.o unicast_client.o \
> + unicast_fsm.o unicast_service.o util.o version.o
>
So the interface.o isn't being added to something like $(CONFIG)?
> OBJECTS = $(OBJ) hwstamp_ctl.o nsm.o phc2sys.o phc_ctl.o pmc.o
> pmc_common.o \
> sysoff.o timemaster.o
>
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH RFC 05/30] Move the network interface into its own header file.
On 2/11/2020 6:03 AM, Richard Cochran wrote:
> Up until now, the users of the interface data structure simply access
> its fields without restriction. This patch takes the first step
> towards abstracting this data structure by giving it a file of its
> very own.
>
> Signed-off-by: Richard Cochran
Reviewed-by: Jacob Keller
> ---
> config.h| 15 +--
> interface.h | 28
> 2 files changed, 29 insertions(+), 14 deletions(-)
> create mode 100644 interface.h
>
> diff --git a/config.h b/config.h
> index e27d3e2..14d2f64 100644
> --- a/config.h
> +++ b/config.h
> @@ -26,25 +26,12 @@
> #include "ds.h"
> #include "dm.h"
> #include "filter.h"
> +#include "interface.h"
> #include "mtab.h"
> #include "transport.h"
> #include "servo.h"
> #include "sk.h"
>
> -#define MAX_IFNAME_SIZE 108 /* = UNIX_PATH_MAX */
> -
> -#if (IF_NAMESIZE > MAX_IFNAME_SIZE)
> -#error if_namesize larger than expected.
> -#endif
> -
> -/** Defines a network interface, with PTP options. */
> -struct interface {
> - STAILQ_ENTRY(interface) list;
> - char name[MAX_IFNAME_SIZE + 1];
> - char ts_label[MAX_IFNAME_SIZE + 1];
> - struct sk_ts_info ts_info;
> -};
> -
Yea this doesn't really belong in config.h at all.
> struct config {
> /* configured interfaces */
> STAILQ_HEAD(interfaces_head, interface) interfaces;
> diff --git a/interface.h b/interface.h
> new file mode 100644
> index 000..61d53a2
> --- /dev/null
> +++ b/interface.h
> @@ -0,0 +1,28 @@
> +/**
> + * @file interface.h
> + * @brief Implements network interface data structures.
> + * @note Copyright (C) 2020 Richard Cochran
> + * @note SPDX-License-Identifier: GPL-2.0+
> + */
> +#ifndef HAVE_INTERFACE_H
> +#define HAVE_INTERFACE_H
> +
> +#include
> +#include "sk.h"
> +
> +#define MAX_IFNAME_SIZE 108 /* = UNIX_PATH_MAX */
> +
> +#if (IF_NAMESIZE > MAX_IFNAME_SIZE)
> +#error if_namesize larger than expected.
> +#endif
> +
> +/** Defines a network interface, with PTP options. */
> +struct interface {
> + STAILQ_ENTRY(interface) list;
> + char name[MAX_IFNAME_SIZE + 1];
> + char ts_label[MAX_IFNAME_SIZE + 1];
> + struct sk_ts_info ts_info;
> +};
> +
> +#endif
> +
>
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH RFC 04/30] utils: Constify the posix clock interface.
On 2/11/2020 6:03 AM, Richard Cochran wrote:
> The function to open a posix clock never modifies the passed in
> string. This patch adds the const keyword to ensure this function
> stays that way.
>
> Signed-off-by: Richard Cochran
Reviewed-by: Jacob Keller
> ---
> util.c | 2 +-
> util.h | 2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/util.c b/util.c
> index e64a93d..43d6224 100644
> --- a/util.c
> +++ b/util.c
> @@ -190,7 +190,7 @@ char *portaddr2str(struct PortAddress *addr)
> return buf;
> }
>
> -clockid_t posix_clock_open(char *device, int *phc_index)
> +clockid_t posix_clock_open(const char *device, int *phc_index)
> {
> struct sk_ts_info ts_info;
> char phc_device[19];
> diff --git a/util.h b/util.h
> index 60d28ac..11e0935 100644
> --- a/util.h
> +++ b/util.h
> @@ -116,7 +116,7 @@ char *portaddr2str(struct PortAddress *addr);
> * @param phc_index Returns the PHC index, if any.
> * @return A valid clock ID on success or CLOCK_INVALID otherwise.
> */
> -clockid_t posix_clock_open(char *device, int *phc_index);
> +clockid_t posix_clock_open(const char *device, int *phc_index);
>
> /**
> * Compare two port identities for equality.
>
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH RFC 03/30] rtnl: Constify the public interface.
On 2/11/2020 6:03 AM, Richard Cochran wrote:
> Three of the rtnl methods never modify the strings passed in. This
> patch adds the const keyword to ensure these functions stay that way.
>
> Signed-off-by: Richard Cochran
Reviewed-by: Jacob Keller
> ---
> rtnl.c | 6 +++---
> rtnl.h | 6 +++---
> 2 files changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/rtnl.c b/rtnl.c
> index 59ed0ec..d9c76d7 100644
> --- a/rtnl.c
> +++ b/rtnl.c
> @@ -87,7 +87,7 @@ static void rtnl_get_ts_device_callback(void *ctx, int
> linkup, int ts_index)
> *dst = ts_index;
> }
>
> -int rtnl_get_ts_device(char *device, char *ts_device)
> +int rtnl_get_ts_device(const char *device, char *ts_device)
> {
> int err, fd;
> int ts_index = -1;
> @@ -112,7 +112,7 @@ no_info:
> return err;
> }
>
> -int rtnl_link_query(int fd, char *device)
> +int rtnl_link_query(int fd, const char *device)
> {
> struct sockaddr_nl sa;
> struct msghdr msg;
> @@ -227,7 +227,7 @@ static int rtnl_linkinfo_parse(int master_index, struct
> rtattr *rta)
> return index;
> }
>
> -int rtnl_link_status(int fd, char *device, rtnl_callback cb, void *ctx)
> +int rtnl_link_status(int fd, const char *device, rtnl_callback cb, void *ctx)
> {
> struct rtattr *tb[IFLA_MAX+1];
> struct ifinfomsg *info = NULL;
> diff --git a/rtnl.h b/rtnl.h
> index f877cd2..c5ea979 100644
> --- a/rtnl.h
> +++ b/rtnl.h
> @@ -37,7 +37,7 @@ int rtnl_close(int fd);
> * at least IF_NAMESIZE bytes long.
> * @return Zero on success, or -1 on error.
> */
> -int rtnl_get_ts_device(char *device, char *ts_device);
> +int rtnl_get_ts_device(const char *device, char *ts_device);
>
> /**
> * Request the link status from the kernel.
> @@ -45,7 +45,7 @@ int rtnl_get_ts_device(char *device, char *ts_device);
> * @param device Interface name. Request all iface's status if set NULL.
> * @return Zero on success, non-zero otherwise.
> */
> -int rtnl_link_query(int fd, char *device);
> +int rtnl_link_query(int fd, const char *device);
>
> /**
> * Read kernel messages looking for a link up/down events.
> @@ -55,7 +55,7 @@ int rtnl_link_query(int fd, char *device);
> * @param ctxPrivate context passed to the callback.
> * @return Zero on success, non-zero otherwise.
> */
> -int rtnl_link_status(int fd, char *device, rtnl_callback cb, void *ctx);
> +int rtnl_link_status(int fd, const char *device, rtnl_callback cb, void
> *ctx);
>
> /**
> * Open a RT netlink socket for monitoring link state.
>
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH RFC 02/30] config: Constify the public interface.
On 2/11/2020 6:03 AM, Richard Cochran wrote:
> The two methods, config_create_interface and config_read, never modify the
> strings passed in. This patch adds the const keyword to ensure these
> functions stay that way.
>
Makes sense. Using const in more places like this is great! It helps
make behavior explicit.
> Signed-off-by: Richard Cochran
Reviewed-by: Jacob Keller
> ---
> config.c | 4 ++--
> config.h | 4 ++--
> 2 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/config.c b/config.c
> index 12eb1f9..65afa70 100644
> --- a/config.c
> +++ b/config.c
> @@ -710,7 +710,7 @@ static struct option *config_alloc_longopts(void)
> return opts;
> }
>
> -int config_read(char *name, struct config *cfg)
> +int config_read(const char *name, struct config *cfg)
> {
> enum config_section current_section = UNKNOWN_SECTION;
> enum parser_result parser_res;
> @@ -817,7 +817,7 @@ parse_error:
> return -2;
> }
>
> -struct interface *config_create_interface(char *name, struct config *cfg)
> +struct interface *config_create_interface(const char *name, struct config
> *cfg)
> {
> struct interface *iface;
>
> diff --git a/config.h b/config.h
> index f237fb2..e27d3e2 100644
> --- a/config.h
> +++ b/config.h
> @@ -60,8 +60,8 @@ struct config {
> STAILQ_HEAD(ucmtab_head, unicast_master_table) unicast_master_tables;
> };
>
> -int config_read(char *name, struct config *cfg);
> -struct interface *config_create_interface(char *name, struct config *cfg);
> +int config_read(const char *name, struct config *cfg);
> +struct interface *config_create_interface(const char *name, struct config
> *cfg);
> void config_destroy(struct config *cfg);
>
> /* New, hash table based methods: */
>
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH RFC 01/30] Group related objects together within the makefile.
On 2/11/2020 6:03 AM, Richard Cochran wrote: > Any program that links to the servo interface must also link with the > implementations of that interface. Similarly, the filter and network > transport interfaces each require their implementations. This patch > re-factors the makefile to reflect this fact in order to simplify > adding new programs making use of these interfaces. > > Signed-off-by: Richard Cochran Makes sense. I probably would have kept the variables together instead of spreading them through the list in alphabetical order, but I see no issue with that. This makes sense and helps prevent possible bugs in the future. Additionally, adding more groups can be done over time in case new modules have similar coupling in the future. Reviewed-by: Jacob Keller > --- > makefile | 32 +--- > 1 file changed, 17 insertions(+), 15 deletions(-) > > diff --git a/makefile b/makefile > index 3397d3e..d58d13a 100644 > --- a/makefile > +++ b/makefile > @@ -23,12 +23,14 @@ VER = -DVER=$(version) > CFLAGS = -Wall $(VER) $(incdefs) $(DEBUG) $(EXTRA_CFLAGS) > LDLIBS = -lm -lrt $(EXTRA_LDFLAGS) > PRG = ptp4l hwstamp_ctl nsm phc2sys phc_ctl pmc timemaster > -OBJ = bmc.o clock.o clockadj.o clockcheck.o config.o designated_fsm.o \ > -e2e_tc.o fault.o filter.o fsm.o hash.o linreg.o mave.o mmedian.o msg.o > ntpshm.o \ > -nullf.o phc.o pi.o port.o port_signaling.o pqueue.o print.o ptp4l.o p2p_tc.o > \ > -raw.o rtnl.o servo.o sk.o stats.o tc.o telecom.o tlv.o transport.o tsproc.o \ > -udp.o udp6.o uds.o unicast_client.o unicast_fsm.o unicast_service.o util.o \ > -version.o > +FILTERS = filter.o mave.o mmedian.o > +SERVOS = linreg.o ntpshm.o nullf.o pi.o servo.o > +TRANSP = raw.o transport.o udp.o udp6.o uds.o > +OBJ = bmc.o clock.o clockadj.o clockcheck.o config.o designated_fsm.o \ > + e2e_tc.o fault.o $(FILTERS) fsm.o hash.o msg.o phc.o port.o > port_signaling.o \ > + pqueue.o print.o ptp4l.o p2p_tc.o rtnl.o $(SERVOS) sk.o stats.o tc.o \ > + $(TRANSP) telecom.o tlv.o tsproc.o unicast_client.o unicast_fsm.o \ > + unicast_service.o util.o version.o > > OBJECTS = $(OBJ) hwstamp_ctl.o nsm.o phc2sys.o phc_ctl.o pmc.o > pmc_common.o \ > sysoff.o timemaster.o > @@ -55,22 +57,22 @@ all: $(PRG) > > ptp4l: $(OBJ) > > -nsm: config.o filter.o hash.o mave.o mmedian.o msg.o nsm.o phc.o print.o > raw.o \ > - rtnl.o sk.o transport.o tlv.o tsproc.o udp.o udp6.o uds.o util.o version.o > +nsm: config.o $(FILTERS) hash.o msg.o nsm.o phc.o print.o \ > + rtnl.o sk.o $(TRANSP) tlv.o tsproc.o util.o version.o > > -pmc: config.o hash.o msg.o phc.o pmc.o pmc_common.o print.o raw.o sk.o tlv.o > \ > - transport.o udp.o udp6.o uds.o util.o version.o > +pmc: config.o hash.o msg.o phc.o pmc.o pmc_common.o print.o sk.o tlv.o \ > + $(TRANSP) util.o version.o > > -phc2sys: clockadj.o clockcheck.o config.o hash.o linreg.o msg.o ntpshm.o \ > - nullf.o phc.o phc2sys.o pi.o pmc_common.o print.o raw.o servo.o sk.o > stats.o \ > - sysoff.o tlv.o transport.o udp.o udp6.o uds.o util.o version.o > +phc2sys: clockadj.o clockcheck.o config.o hash.o msg.o \ > + phc.o phc2sys.o pmc_common.o print.o $(SERVOS) sk.o stats.o \ > + sysoff.o tlv.o $(TRANSP) util.o version.o > > hwstamp_ctl: hwstamp_ctl.o version.o > > phc_ctl: phc_ctl.o phc.o sk.o util.o clockadj.o sysoff.o print.o version.o > > -snmp4lptp: config.o hash.o msg.o phc.o pmc_common.o print.o raw.o sk.o \ > - snmp4lptp.o tlv.o transport.o udp.o udp6.o uds.o util.o > +snmp4lptp: config.o hash.o msg.o phc.o pmc_common.o print.o sk.o \ > + snmp4lptp.o tlv.o $(TRANSP) util.o > $(CC) $^ $(LDFLAGS) $(LOADLIBES) $(LDLIBS) $(snmplib) -o $@ > > snmp4lptp.o: snmp4lptp.c > ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH RFC 00/30] Convert open coded interface into proper module.
On 2/11/2020 6:03 AM, Richard Cochran wrote: > Using gcc8 and -O2, the compiler emits an annoying false positive > warning. Since I want to have -Werror, I went about fixing it. Start > pulling on a thread, and ... > Excellent goal! > The 'struct interface' is wide open to its users, and each user > fiddles with the fields in a different way. The cure is, of course, > to hide the implementation details in a source module. This task > turned out to be harder than it looked at first. > Yea this can be quite challenging... > - Patch #1 refactors the makefile a bit (this will be useful in a series to > follow) > - Patches 2-3 clean up interfaces accepting character strings as parameters. > - Patch #5 introduces the "interface" module. > - Patches 6-23 convert all the users to proper methods, one method at a time. > - Patches 24-27 add create/destroy methods. > - Patch #28 hides the implementation of the struct. > - Patch #29 _finally_ fixes the compiler warning. > - Patch #30 removes left over crud. > Nice. A lot of patches, but they sound reasonable. Will review them this afternoon. Regards, Jake > Thanks for your review, > > Richard Cochran (30): > Group related objects together within the makefile. > config: Constify the public interface. > rtnl: Constify the public interface. > utils: Constify the posix clock interface. > Move the network interface into its own header file. > interface: Introduce an access method for the name field. > Convert call sites to the proper method for getting interface names. > interface: Introduce an access method for the time stamping label. > Convert call sites to the proper method for getting interface labels. > interface: Introduce a method to get the time stamping information. > Convert call sites to the proper method for getting time stamp > information. > interface: Introduce a method to initialize the time stamping label. > Convert call sites to the proper method for initializing the time > stamping label. > interface: Introduce a method to set the name. > Convert call sites to the proper method for setting the name. > interface: Introduce a method to set the time stamping label. > Convert call sites to the proper method for setting the time stamping > label. > interface: Introduce a method to get the PHC index. > Convert call sites to the proper method for getting the PHC index. > interface: Introduce a method to test the time stamping information > validity. > Convert call sites to the proper method for testing time stamp info > validity. > interface: Introduce a method to test supported time stamping modes. > Convert call sites to the proper method for testing time stamping > modes. > interface: Introduce methods to create and destroy instances. > clock: Use the proper create/destroy API for network interfaces. > config: Use the proper create/destroy API for network interfaces. > pmc: Use the proper create/destroy API for network interfaces. > interface: Hide the implementation details. > interface: Silence warning from gcc version 8. > interface: Remove obsolete method. > > clock.c| 61 > config.c | 26 -- > config.h | 19 ++ > interface.c| 78 + > interface.h| 94 ++ > makefile | 32 + > nsm.c | 18 ++ > pmc_common.c | 19 +- > port.c | 59 +-- > port_private.h | 2 +- > raw.c | 5 +-- > rtnl.c | 6 ++-- > rtnl.h | 8 +++-- > udp.c | 4 +-- > udp6.c | 4 +-- > uds.c | 8 ++--- > util.c | 2 +- > util.h | 2 +- > 18 files changed, 316 insertions(+), 131 deletions(-) > create mode 100644 interface.c > create mode 100644 interface.h > ___ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
