On 8/1/2020 10:46 AM, Vladimir Oltean wrote:
> Eliminate the ad-hoc use of global variables in the ts2phc program by
> introducing one data structure that incorporates them. This might make
> the code more understandable to people coming from a kernel background,
> since it resembles the type of data organization used there. It is also
> now closer to the data organization of phc2sys, a similar program in
> both purpose and implementation.
> 
> Signed-off-by: Vladimir Oltean <olte...@gmail.com>

This patch is a bit difficult to read because a number of things get
moved at once. Hoever, I think it's a significant improvement on the
current state of things, and trying to do this transition piece-meal
would be difficult.

Avoiding global variables here is definitely an improvement, as it makes
it easier to think about who has access to what data.

Reviewed-by: Jacob Keller <jacob.e.kel...@intel.com>

> ---
>  ts2phc.c       |  64 ++++++++++++-----------
>  ts2phc.h       |  37 ++++++++++++++
>  ts2phc_slave.c | 136 ++++++++++++++++++++++++++++---------------------
>  ts2phc_slave.h |  10 ++--
>  4 files changed, 153 insertions(+), 94 deletions(-)
>  create mode 100644 ts2phc.h
> 
> diff --git a/ts2phc.c b/ts2phc.c
> index 23428586ef66..0be7b18bda71 100644
> --- a/ts2phc.c
> +++ b/ts2phc.c
> @@ -11,22 +11,21 @@
>  #include "config.h"
>  #include "interface.h"
>  #include "print.h"
> -#include "ts2phc_master.h"
> -#include "ts2phc_slave.h"
> +#include "ts2phc.h"
>  #include "version.h"
>  
>  struct interface {
>       STAILQ_ENTRY(interface) list;
>  };
>  
> -static void ts2phc_cleanup(struct config *cfg, struct ts2phc_master *master)
> +static void ts2phc_cleanup(struct ts2phc_private *priv)
>  {
> -     ts2phc_slave_cleanup();
> -     if (master) {
> -             ts2phc_master_destroy(master);
> +     ts2phc_slave_cleanup(priv);
> +     if (priv->master) {
> +             ts2phc_master_destroy(priv->master);
>       }
> -     if (cfg) {
> -             config_destroy(cfg);
> +     if (priv->cfg) {
> +             config_destroy(priv->cfg);
>       }
>  }
>  
> @@ -56,8 +55,8 @@ static void usage(char *progname)
>  int main(int argc, char *argv[])
>  {
>       int c, err = 0, have_slave = 0, index, print_level;
> -     struct ts2phc_master *master = NULL;
>       enum ts2phc_master_type pps_type;
> +     struct ts2phc_private priv = {0};
>       char *config = NULL, *progname;
>       const char *pps_source = NULL;
>       struct config *cfg = NULL;
> @@ -68,7 +67,7 @@ int main(int argc, char *argv[])
>  
>       cfg = config_create();
>       if (!cfg) {
> -             ts2phc_cleanup(cfg, master);
> +             ts2phc_cleanup(&priv);
>               return -1;
>       }
>  
> @@ -81,14 +80,14 @@ int main(int argc, char *argv[])
>               switch (c) {
>               case 0:
>                       if (config_parse_option(cfg, opts[index].name, optarg)) 
> {
> -                             ts2phc_cleanup(cfg, master);
> +                             ts2phc_cleanup(&priv);
>                               return -1;
>                       }
>                       break;
>               case 'c':
>                       if (!config_create_interface(optarg, cfg)) {
>                               fprintf(stderr, "failed to add slave\n");
> -                             ts2phc_cleanup(cfg, master);
> +                             ts2phc_cleanup(&priv);
>                               return -1;
>                       }
>                       have_slave = 1;
> @@ -99,7 +98,7 @@ int main(int argc, char *argv[])
>               case 'l':
>                       if (get_arg_val_i(c, optarg, &print_level,
>                                         PRINT_LEVEL_MIN, PRINT_LEVEL_MAX)) {
> -                             ts2phc_cleanup(cfg, master);
> +                             ts2phc_cleanup(&priv);
>                               return -1;
>                       }
>                       config_set_int(cfg, "logging_level", print_level);
> @@ -116,29 +115,29 @@ int main(int argc, char *argv[])
>               case 's':
>                       if (pps_source) {
>                               fprintf(stderr, "too many PPS sources\n");
> -                             ts2phc_cleanup(cfg, master);
> +                             ts2phc_cleanup(&priv);
>                               return -1;
>                       }
>                       pps_source = optarg;
>                       break;
>               case 'v':
> -                     ts2phc_cleanup(cfg, master);
> +                     ts2phc_cleanup(&priv);
>                       version_show(stdout);
>                       return 0;
>               case 'h':
> -                     ts2phc_cleanup(cfg, master);
> +                     ts2phc_cleanup(&priv);
>                       usage(progname);
>                       return -1;
>               case '?':
>               default:
> -                     ts2phc_cleanup(cfg, master);
> +                     ts2phc_cleanup(&priv);
>                       usage(progname);
>                       return -1;
>               }
>       }
>       if (config && (c = config_read(config, cfg))) {
>               fprintf(stderr, "failed to read config\n");
> -             ts2phc_cleanup(cfg, master);
> +             ts2phc_cleanup(&priv);
>               return -1;
>       }
>       print_set_progname(progname);
> @@ -147,18 +146,21 @@ int main(int argc, char *argv[])
>       print_set_syslog(config_get_int(cfg, NULL, "use_syslog"));
>       print_set_level(config_get_int(cfg, NULL, "logging_level"));
>  
> +     STAILQ_INIT(&priv.slaves);
> +     priv.cfg = cfg;
> +
>       STAILQ_FOREACH(iface, &cfg->interfaces, list) {
>               if (1 == config_get_int(cfg, interface_name(iface), 
> "ts2phc.master")) {
>                       if (pps_source) {
>                               fprintf(stderr, "too many PPS sources\n");
> -                             ts2phc_cleanup(cfg, master);
> +                             ts2phc_cleanup(&priv);
>                               return -1;
>                       }
>                       pps_source = interface_name(iface);
>               } else {
> -                     if (ts2phc_slave_add(cfg, interface_name(iface))) {
> +                     if (ts2phc_slave_add(&priv, interface_name(iface))) {
>                               fprintf(stderr, "failed to add slave\n");
> -                             ts2phc_cleanup(cfg, master);
> +                             ts2phc_cleanup(&priv);
>                               return -1;
>                       }
>                       have_slave = 1;
> @@ -166,19 +168,19 @@ int main(int argc, char *argv[])
>       }
>       if (!have_slave) {
>               fprintf(stderr, "no slave clocks specified\n");
> -             ts2phc_cleanup(cfg, master);
> +             ts2phc_cleanup(&priv);
>               usage(progname);
>               return -1;
>       }
>       if (!pps_source) {
>               fprintf(stderr, "no PPS source specified\n");
> -             ts2phc_cleanup(cfg, master);
> +             ts2phc_cleanup(&priv);
>               usage(progname);
>               return -1;
>       }
> -     if (ts2phc_slave_arm()) {
> -             fprintf(stderr, "failed to arm slaves\n");
> -             ts2phc_cleanup(cfg, master);
> +     if (ts2phc_slaves_init(&priv)) {
> +             fprintf(stderr, "failed to initialize slaves\n");
> +             ts2phc_cleanup(&priv);
>               return -1;
>       }
>  
> @@ -189,21 +191,21 @@ int main(int argc, char *argv[])
>       } else {
>               pps_type = TS2PHC_MASTER_PHC;
>       }
> -     master = ts2phc_master_create(cfg, pps_source, pps_type);
> -     if (!master) {
> +     priv.master = ts2phc_master_create(cfg, pps_source, pps_type);
> +     if (!priv.master) {
>               fprintf(stderr, "failed to create master\n");
> -             ts2phc_cleanup(cfg, master);
> +             ts2phc_cleanup(&priv);
>               return -1;
>       }
>  
>       while (is_running()) {
> -             err = ts2phc_slave_poll(master);
> +             err = ts2phc_slave_poll(&priv);
>               if (err) {
>                       pr_err("poll failed");
>                       break;
>               }
>       }
>  
> -     ts2phc_cleanup(cfg, master);
> +     ts2phc_cleanup(&priv);
>       return err;
>  }
> diff --git a/ts2phc.h b/ts2phc.h
> new file mode 100644
> index 000000000000..a5d50fc7e3be
> --- /dev/null
> +++ b/ts2phc.h
> @@ -0,0 +1,37 @@
> +/**
> + * @file ts2phc.h
> + * @brief Structure definitions for ts2phc
> + * @note Copyright 2020 Vladimir Oltean <olte...@gmail.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License along
> + * with this program; if not, write to the Free Software Foundation, Inc.,
> + * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
> + */
> +
> +#ifndef HAVE_TS2PHC_H
> +#define HAVE_TS2PHC_H
> +
> +struct ts2phc_slave_array;
> +
> +struct ts2phc_private {
> +     struct ts2phc_master *master;
> +     STAILQ_HEAD(slave_ifaces_head, ts2phc_slave) slaves;
> +     unsigned int n_slaves;
> +     struct ts2phc_slave_array *polling_array;
> +     struct config *cfg;
> +};
> +
> +#include "ts2phc_master.h"
> +#include "ts2phc_slave.h"
> +
> +#endif
> diff --git a/ts2phc_slave.c b/ts2phc_slave.c
> index 749efe5f81ba..7b4fbfa70440 100644
> --- a/ts2phc_slave.c
> +++ b/ts2phc_slave.c
> @@ -21,8 +21,7 @@
>  #include "phc.h"
>  #include "print.h"
>  #include "servo.h"
> -#include "ts2phc_master.h"
> -#include "ts2phc_slave.h"
> +#include "ts2phc.h"
>  #include "util.h"
>  
>  #define NS_PER_SEC           1000000000LL
> @@ -47,7 +46,7 @@ struct ts2phc_slave {
>  struct ts2phc_slave_array {
>       struct ts2phc_slave **slave;
>       struct pollfd *pfd;
> -} polling_array;
> +};
>  
>  struct ts2phc_source_timestamp {
>       struct timespec ts;
> @@ -65,49 +64,55 @@ static enum extts_result ts2phc_slave_offset(struct 
> ts2phc_slave *slave,
>                                            int64_t *offset,
>                                            uint64_t *local_ts);
>  
> -static STAILQ_HEAD(slave_ifaces_head, ts2phc_slave) ts2phc_slaves =
> -     STAILQ_HEAD_INITIALIZER(ts2phc_slaves);
> -
> -static unsigned int ts2phc_n_slaves;
> -
> -static int ts2phc_slave_array_create(void)
> +static int ts2phc_slave_array_create(struct ts2phc_private *priv)
>  {
> +     struct ts2phc_slave_array *polling_array;
>       struct ts2phc_slave *slave;
>       unsigned int i;
>  
> -     if (polling_array.slave) {
> -             return 0;
> +     polling_array = malloc(sizeof(*polling_array));
> +     if (!polling_array) {
> +             pr_err("low memory");
> +             return -1;
>       }
> -     polling_array.slave = malloc(ts2phc_n_slaves * 
> sizeof(*polling_array.slave));
> -     if (!polling_array.slave) {
> +
> +     polling_array->slave = malloc(priv->n_slaves *
> +                                   sizeof(*polling_array->slave));
> +     if (!polling_array->slave) {
>               pr_err("low memory");
>               return -1;
>       }
> -     polling_array.pfd = malloc(ts2phc_n_slaves * 
> sizeof(*polling_array.pfd));
> -     if (!polling_array.pfd) {
> +     polling_array->pfd = malloc(priv->n_slaves *
> +                                 sizeof(*polling_array->pfd));
> +     if (!polling_array->pfd) {
>               pr_err("low memory");
> -             free(polling_array.slave);
> -             polling_array.slave = NULL;
> +             free(polling_array->slave);
> +             polling_array->slave = NULL;
>               return -1;
>       }
>       i = 0;
> -     STAILQ_FOREACH(slave, &ts2phc_slaves, list) {
> -             polling_array.slave[i] = slave;
> +     STAILQ_FOREACH(slave, &priv->slaves, list) {
> +             polling_array->slave[i] = slave;
>               i++;
>       }
> -     for (i = 0; i < ts2phc_n_slaves; i++) {
> -             polling_array.pfd[i].events = POLLIN | POLLPRI;
> -             polling_array.pfd[i].fd = polling_array.slave[i]->fd;
> +     for (i = 0; i < priv->n_slaves; i++) {
> +             polling_array->pfd[i].events = POLLIN | POLLPRI;
> +             polling_array->pfd[i].fd = polling_array->slave[i]->fd;
>       }
> +
> +     priv->polling_array = polling_array;
> +
>       return 0;
>  }
>  
> -static void ts2phc_slave_array_destroy(void)
> +static void ts2phc_slave_array_destroy(struct ts2phc_private *priv)
>  {
> -     free(polling_array.slave);
> -     free(polling_array.pfd);
> -     polling_array.slave = NULL;
> -     polling_array.pfd = NULL;
> +     struct ts2phc_slave_array *polling_array = priv->polling_array;
> +
> +     free(polling_array->slave);
> +     free(polling_array->pfd);
> +     polling_array->slave = NULL;
> +     polling_array->pfd = NULL;
>  }
>  
>  static int ts2phc_slave_clear_fifo(struct ts2phc_slave *slave)
> @@ -143,9 +148,10 @@ static int ts2phc_slave_clear_fifo(struct ts2phc_slave 
> *slave)
>       return 0;
>  }
>  
> -static struct ts2phc_slave *ts2phc_slave_create(struct config *cfg, const 
> char *device)
> +static struct ts2phc_slave *ts2phc_slave_create(struct ts2phc_private *priv,
> +                                             const char *device)
>  {
> -     enum servo_type servo = config_get_int(cfg, NULL, "clock_servo");
> +     enum servo_type servo = config_get_int(priv->cfg, NULL, "clock_servo");
>       int err, fadj, junk, max_adj, pulsewidth;
>       struct ptp_extts_request extts;
>       struct ts2phc_slave *slave;
> @@ -161,13 +167,18 @@ static struct ts2phc_slave *ts2phc_slave_create(struct 
> config *cfg, const char *
>               free(slave);
>               return NULL;
>       }
> -     slave->pin_desc.index = config_get_int(cfg, device, "ts2phc.pin_index");
> +     slave->pin_desc.index = config_get_int(priv->cfg, device,
> +                                            "ts2phc.pin_index");
>       slave->pin_desc.func = PTP_PF_EXTTS;
> -     slave->pin_desc.chan = config_get_int(cfg, device, "ts2phc.channel");
> -     slave->polarity = config_get_int(cfg, device, "ts2phc.extts_polarity");
> -     slave->correction = config_get_int(cfg, device, 
> "ts2phc.extts_correction");
> -
> -     pulsewidth = config_get_int(cfg, device, "ts2phc.pulsewidth");
> +     slave->pin_desc.chan = config_get_int(priv->cfg, device,
> +                                           "ts2phc.channel");
> +     slave->polarity = config_get_int(priv->cfg, device,
> +                                      "ts2phc.extts_polarity");
> +     slave->correction = config_get_int(priv->cfg, device,
> +                                        "ts2phc.extts_correction");
> +
> +     pulsewidth = config_get_int(priv->cfg, device,
> +                                 "ts2phc.pulsewidth");
>       pulsewidth /= 2;
>       slave->ignore_upper = 1000000000 - pulsewidth;
>       slave->ignore_lower = pulsewidth;
> @@ -177,7 +188,7 @@ static struct ts2phc_slave *ts2phc_slave_create(struct 
> config *cfg, const char *
>               pr_err("failed to open clock");
>               goto no_posix_clock;
>       }
> -     slave->no_adj = config_get_int(cfg, NULL, "free_running");
> +     slave->no_adj = config_get_int(priv->cfg, NULL, "free_running");
>       slave->fd = CLOCKID_TO_FD(slave->clk);
>  
>       pr_debug("PHC slave %s has ptp index %d", device, junk);
> @@ -190,7 +201,7 @@ static struct ts2phc_slave *ts2phc_slave_create(struct 
> config *cfg, const char *
>  
>       max_adj = phc_max_adj(slave->clk);
>  
> -     slave->servo = servo_create(cfg, servo, -fadj, max_adj, 0);
> +     slave->servo = servo_create(priv->cfg, servo, -fadj, max_adj, 0);
>       if (!slave->servo) {
>               pr_err("failed to create servo");
>               goto no_servo;
> @@ -346,28 +357,28 @@ static enum extts_result ts2phc_slave_offset(struct 
> ts2phc_slave *slave,
>  
>  /* public methods */
>  
> -int ts2phc_slave_add(struct config *cfg, const char *name)
> +int ts2phc_slave_add(struct ts2phc_private *priv, const char *name)
>  {
>       struct ts2phc_slave *slave;
>  
>       /* Create each interface only once. */
> -     STAILQ_FOREACH(slave, &ts2phc_slaves, list) {
> +     STAILQ_FOREACH(slave, &priv->slaves, list) {
>               if (0 == strcmp(name, slave->name)) {
>                       return 0;
>               }
>       }
> -     slave = ts2phc_slave_create(cfg, name);
> +     slave = ts2phc_slave_create(priv, name);
>       if (!slave) {
>               pr_err("failed to create slave");
>               return -1;
>       }
> -     STAILQ_INSERT_TAIL(&ts2phc_slaves, slave, list);
> -     ts2phc_n_slaves++;
> +     STAILQ_INSERT_TAIL(&priv->slaves, slave, list);
> +     priv->n_slaves++;
>  
>       return 0;
>  }
>  
> -int ts2phc_slave_arm(void)
> +int ts2phc_slave_arm(struct ts2phc_private *priv)
>  {
>       struct ptp_extts_request extts;
>       struct ts2phc_slave *slave;
> @@ -375,7 +386,7 @@ int ts2phc_slave_arm(void)
>  
>       memset(&extts, 0, sizeof(extts));
>  
> -     STAILQ_FOREACH(slave, &ts2phc_slaves, list) {
> +     STAILQ_FOREACH(slave, &priv->slaves, list) {
>               extts.index = slave->pin_desc.chan;
>               extts.flags = slave->polarity | PTP_ENABLE_FEATURE;
>               err = ioctl(slave->fd, PTP_EXTTS_REQUEST2, &extts);
> @@ -387,29 +398,38 @@ int ts2phc_slave_arm(void)
>       return 0;
>  }
>  
> -void ts2phc_slave_cleanup(void)
> +int ts2phc_slaves_init(struct ts2phc_private *priv)
> +{
> +     int err;
> +
> +     err = ts2phc_slave_array_create(priv);
> +     if (err)
> +             return err;
> +
> +     return ts2phc_slave_arm(priv);
> +}
> +
> +void ts2phc_slave_cleanup(struct ts2phc_private *priv)
>  {
>       struct ts2phc_slave *slave;
>  
> -     ts2phc_slave_array_destroy();
> +     ts2phc_slave_array_destroy(priv);
>  
> -     while ((slave = STAILQ_FIRST(&ts2phc_slaves))) {
> -             STAILQ_REMOVE_HEAD(&ts2phc_slaves, list);
> +     while ((slave = STAILQ_FIRST(&priv->slaves))) {
> +             STAILQ_REMOVE_HEAD(&priv->slaves, list);
>               ts2phc_slave_destroy(slave);
> -             ts2phc_n_slaves--;
> +             priv->n_slaves--;
>       }
>  }
>  
> -int ts2phc_slave_poll(struct ts2phc_master *master)
> +int ts2phc_slave_poll(struct ts2phc_private *priv)
>  {
> +     struct ts2phc_slave_array *polling_array = priv->polling_array;
>       struct ts2phc_source_timestamp source_ts;
>       unsigned int i;
>       int cnt, err;
>  
> -     if (ts2phc_slave_array_create()) {
> -             return -1;
> -     }
> -     cnt = poll(polling_array.pfd, ts2phc_n_slaves, 2000);
> +     cnt = poll(polling_array->pfd, priv->n_slaves, 2000);
>       if (cnt < 0) {
>               if (EINTR == errno) {
>                       return 0;
> @@ -422,12 +442,12 @@ int ts2phc_slave_poll(struct ts2phc_master *master)
>               return 0;
>       }
>  
> -     err = ts2phc_master_getppstime(master, &source_ts.ts);
> +     err = ts2phc_master_getppstime(priv->master, &source_ts.ts);
>       source_ts.valid = err ? false : true;
>  
> -     for (i = 0; i < ts2phc_n_slaves; i++) {
> -             if (polling_array.pfd[i].revents & (POLLIN|POLLPRI)) {
> -                     ts2phc_slave_event(polling_array.slave[i], source_ts);
> +     for (i = 0; i < priv->n_slaves; i++) {
> +             if (polling_array->pfd[i].revents & (POLLIN|POLLPRI)) {
> +                     ts2phc_slave_event(polling_array->slave[i], source_ts);
>               }
>       }
>       return 0;
> diff --git a/ts2phc_slave.h b/ts2phc_slave.h
> index 2de5ab7dd0ff..1bad9d297799 100644
> --- a/ts2phc_slave.h
> +++ b/ts2phc_slave.h
> @@ -7,14 +7,14 @@
>  #ifndef HAVE_TS2PHC_SLAVE_H
>  #define HAVE_TS2PHC_SLAVE_H
>  
> -#include "ts2phc_master.h"
> +#include "ts2phc.h"
>  
> -int ts2phc_slave_add(struct config *cfg, const char *name);
> +int ts2phc_slave_add(struct ts2phc_private *priv, const char *name);
>  
> -int ts2phc_slave_arm(void);
> +int ts2phc_slaves_init(struct ts2phc_private *priv);
>  
> -void ts2phc_slave_cleanup(void);
> +void ts2phc_slave_cleanup(struct ts2phc_private *priv);
>  
> -int ts2phc_slave_poll(struct ts2phc_master *master);
> +int ts2phc_slave_poll(struct ts2phc_private *priv);
>  
>  #endif
> 


_______________________________________________
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel

Reply via email to