[Linuxptp-devel] [PATCH 1/2] ptp4l: ignore empty lines and comments in config.

[Miroslav Lichvar]

Signed-off-by: Miroslav Lichvar <***@redhat.com>
---
config.c | 15 +++++++++++++--
ptp4l.8 | 3 ++-
2 files changed, 15 insertions(+), 3 deletions(-)

diff --git a/config.c b/config.c
index 6444108..e0d87d5 100644
--- a/config.c
+++ b/config.c
@@ -18,6 +18,7 @@
*/
#include <stdio.h>
#include <string.h>
+#include <ctype.h>
#include "config.h"
#include "ether.h"
#include "print.h"
@@ -318,7 +319,7 @@ int config_read(char *name, struct config *cfg)
{
enum config_section current_section = GLOBAL_SECTION;
FILE *fp;
- char line[1024];
+ char buf[1024], *line;
int current_port;

fp = 0 == strncmp(name, "-", 2) ? stdin : fopen(name, "r");
@@ -328,7 +329,17 @@ int config_read(char *name, struct config *cfg)
return -1;
}

- while (fgets(line, sizeof(line), fp)) {
+ while (fgets(buf, sizeof(buf), fp)) {
+ line = buf;
+
+ /* skip whitespace characters */
+ while (isspace(line[0]))
+ line++;
+
+ /* ignore empty lines and comments */
+ if (line[0] == '#' || line[0] == '\n' || line[0] == '\0')
+ continue;
+
if (scan_mode(line, &current_section) ) {
if (current_section == PORT_SECTION) {
char port[17];
diff --git a/ptp4l.8 b/ptp4l.8
index f496ede..1114b34 100644
--- a/ptp4l.8
+++ b/ptp4l.8
@@ -113,7 +113,8 @@ Display a help message.
The configuration file is divided into sections. Each section starts with a
line containing its name enclosed in brackets and it follows with settings.
Each setting is placed on a separate line, it contains the name of the
-option and the value separated by whitespace characters.
+option and the value separated by whitespace characters. Empty lines and lines
+starting with # are ignored.

The global section (indicated as
.BR [global] )

[Miroslav Lichvar]

- fail on unknown options and other syntax errors
- fail with more than MAX_PORTS ports
- remove implicit global section
- add error messages

Signed-off-by: Miroslav Lichvar <***@redhat.com>
---
config.c | 56 ++++++++++++++++++++++++++++++++++++++++++++------------
ptp4l.c | 13 ++++++++++---
2 files changed, 54 insertions(+), 15 deletions(-)

diff --git a/config.c b/config.c
index e0d87d5..3d8024c 100644
--- a/config.c
+++ b/config.c
@@ -99,7 +99,7 @@ static int scan_pod(const char *s, struct port_defaults *pod)
return 0;
}

-static void scan_port_line(const char *s, struct config *cfg, int p)
+static int scan_port_line(const char *s, struct config *cfg, int p)
{
char string[1024];

@@ -121,6 +121,9 @@ static void scan_port_line(const char *s, struct config *cfg, int p)

cfg->iface[p].transport = TRANS_UDP_IPV6;

+ else
+ return 0;
+
} else if (1 == sscanf(s, " delay_mechanism %1023s", string)) {

if (0 == strcasecmp("Auto", string))
@@ -135,10 +138,15 @@ static void scan_port_line(const char *s, struct config *cfg, int p)

cfg->iface[p].dm = DM_P2P;

- }
+ else
+ return 0;
+ } else
+ return 0;
+
+ return 1;
}

-static void scan_global_line(const char *s, struct config *cfg)
+static int scan_global_line(const char *s, struct config *cfg)
{
double df;
int i, val, cfg_ignore = cfg->cfg_ignore;
@@ -269,6 +277,9 @@ static void scan_global_line(const char *s, struct config *cfg)
else if (0 == strcasecmp("legacy", string))

cfg->timestamping = TS_LEGACY_HW;
+
+ else
+ return 0;
}

} else if (1 == sscanf(s, " delay_mechanism %1023s", string)) {
@@ -286,6 +297,9 @@ static void scan_global_line(const char *s, struct config *cfg)
else if (0 == strcasecmp("Auto", string))

cfg->dm = DM_AUTO;
+
+ else
+ return 0;
}

} else if (1 == sscanf(s, " network_transport %1023s", string)) {
@@ -304,6 +318,8 @@ static void scan_global_line(const char *s, struct config *cfg)

cfg->transport = TRANS_IEEE_802_3;

+ else
+ return 0;
}

} else if (1 == sscanf(s, " clock_servo %1023s", string)) {
@@ -312,15 +328,20 @@ static void scan_global_line(const char *s, struct config *cfg)

cfg->clock_servo = CLOCK_SERVO_PI;

- }
+ else
+ return 0;
+ } else
+ return 0;
+
+ return 1;
}

int config_read(char *name, struct config *cfg)
{
- enum config_section current_section = GLOBAL_SECTION;
+ enum config_section current_section = UNKNOWN_SECTION;
FILE *fp;
char buf[1024], *line;
- int current_port;
+ int current_port, line_num;

fp = 0 == strncmp(name, "-", 2) ? stdin : fopen(name, "r");

@@ -329,7 +350,7 @@ int config_read(char *name, struct config *cfg)
return -1;
}

- while (fgets(buf, sizeof(buf), fp)) {
+ for (line_num = 1; fgets(buf, sizeof(buf), fp); line_num++) {
line = buf;

/* skip whitespace characters */
@@ -344,12 +365,13 @@ int config_read(char *name, struct config *cfg)
if (current_section == PORT_SECTION) {
char port[17];
if (1 != sscanf(line, " %16s", port)) {
- current_section = UNKNOWN_SECTION;
- continue;
+ fprintf(stderr, "could not parse port name on line %d\n",
+ line_num);
+ return -2;
}
current_port = config_create_interface(port, cfg);
if (current_port < 0) {
- return -1;
+ return -2;
}
}
continue;
@@ -357,11 +379,20 @@ int config_read(char *name, struct config *cfg)

switch(current_section) {
case GLOBAL_SECTION:
- scan_global_line(line, cfg);
+ if (!scan_global_line(line, cfg)) {
+ fprintf(stderr, "syntax error in global section on line %d\n", line_num);
+ return -2;
+ }
break;
case PORT_SECTION:
- scan_port_line(line, cfg, current_port);
+ if (!scan_port_line(line, cfg, current_port)) {
+ fprintf(stderr, "syntax error in port section on line %d\n", line_num);
+ return -2;
+ }
break;
+ case UNKNOWN_SECTION:
+ fprintf(stderr, "line %d not in section\n", line_num);
+ return -2;
default:
continue;
}
@@ -378,6 +409,7 @@ int config_create_interface(char *name, struct config *cfg)
int i;

if (cfg->nports >= MAX_PORTS) {
+ fprintf(stderr, "more than %d ports specified\n", MAX_PORTS);
return -1;
}

diff --git a/ptp4l.c b/ptp4l.c
index bc2bc24..560d6d6 100644
--- a/ptp4l.c
+++ b/ptp4l.c
@@ -229,9 +229,16 @@ int main(int argc, char *argv[])
}
}

- if (config && config_read(config, &cfg_settings)) {
- fprintf(stderr, "failed to read configuration file\n");
- return -1;
+ if (config && (c = config_read(config, &cfg_settings))) {
+ switch (c) {
+ case -1:
+ fprintf(stderr, "failed to read configuration file\n");
+ break;
+ case -2:
+ fprintf(stderr, "failed to parse configuration file\n");
+ break;
+ }
+ return c;
}
if (ds->slaveOnly) {
ds->clockQuality.clockClass = 255;

[Keller, Jacob E]

I have a few comments inline. General comments, would it be possible to explicitly print out error messages for the main sections if it knows the option but the value doesn't match?

For example if you set timestamping mode and that value isn't recognized could we print out a message "%s is an unrecognized value for %s option"?

Thanks

-----Original Message-----
Sent: Tuesday, November 06, 2012 10:00 AM
Subject: [Linuxptp-devel] [PATCH 2/2] ptp4l: make config parser strict.
- fail on unknown options and other syntax errors
- fail with more than MAX_PORTS ports
- remove implicit global section
- add error messages
---
config.c | 56 ++++++++++++++++++++++++++++++++++++++++++++------------
ptp4l.c | 13 ++++++++++---
2 files changed, 54 insertions(+), 15 deletions(-)
diff --git a/config.c b/config.c
index e0d87d5..3d8024c 100644
--- a/config.c
+++ b/config.c
@@ -99,7 +99,7 @@ static int scan_pod(const char *s, struct port_defaults *pod)
return 0;
}
-static void scan_port_line(const char *s, struct config *cfg, int p)
+static int scan_port_line(const char *s, struct config *cfg, int p)
{
char string[1024];
@@ -121,6 +121,9 @@ static void scan_port_line(const char *s, struct config *cfg, int p)
cfg->iface[p].transport = TRANS_UDP_IPV6;
+ else
+ return 0;
+

usually return 0 is for success, non-zero for failure. Any reason you chose otherwise here?

} else if (1 == sscanf(s, " delay_mechanism %1023s", string)) {
if (0 == strcasecmp("Auto", string))
@@ -135,10 +138,15 @@ static void scan_port_line(const char *s, struct config *cfg, int p)
cfg->iface[p].dm = DM_P2P;
- }
+ else
+ return 0;
+ } else
+ return 0;
+
+ return 1;
}
-static void scan_global_line(const char *s, struct config *cfg)
+static int scan_global_line(const char *s, struct config *cfg)
{
double df;
int i, val, cfg_ignore = cfg->cfg_ignore;
@@ -269,6 +277,9 @@ static void scan_global_line(const char *s, struct config *cfg)
else if (0 == strcasecmp("legacy", string))
cfg->timestamping = TS_LEGACY_HW;
+
+ else
+ return 0;
}
} else if (1 == sscanf(s, " delay_mechanism %1023s", string)) {
@@ -286,6 +297,9 @@ static void scan_global_line(const char *s, struct config *cfg)
else if (0 == strcasecmp("Auto", string))
cfg->dm = DM_AUTO;
+
+ else
+ return 0;
}
} else if (1 == sscanf(s, " network_transport %1023s", string)) {
@@ -304,6 +318,8 @@ static void scan_global_line(const char *s, struct config *cfg)
cfg->transport = TRANS_IEEE_802_3;
+ else
+ return 0;
}
} else if (1 == sscanf(s, " clock_servo %1023s", string)) {
@@ -312,15 +328,20 @@ static void scan_global_line(const char *s, struct config *cfg)
cfg->clock_servo = CLOCK_SERVO_PI;
- }
+ else
+ return 0;
+ } else
+ return 0;
+
+ return 1;
}
int config_read(char *name, struct config *cfg)
{
- enum config_section current_section = GLOBAL_SECTION;
+ enum config_section current_section = UNKNOWN_SECTION;
FILE *fp;
char buf[1024], *line;
- int current_port;
+ int current_port, line_num;
fp = 0 == strncmp(name, "-", 2) ? stdin : fopen(name, "r");
@@ -329,7 +350,7 @@ int config_read(char *name, struct config *cfg)
return -1;
}
- while (fgets(buf, sizeof(buf), fp)) {
+ for (line_num = 1; fgets(buf, sizeof(buf), fp); line_num++) {
line = buf;
/* skip whitespace characters */
@@ -344,12 +365,13 @@ int config_read(char *name, struct config *cfg)
if (current_section == PORT_SECTION) {
char port[17];
if (1 != sscanf(line, " %16s", port)) {
- current_section = UNKNOWN_SECTION;
- continue;
+ fprintf(stderr, "could not parse port
name on line %d\n",
+ line_num);
+ return -2;

Maybe print out the line that caused the failure?

}
current_port = config_create_interface(port,
cfg);
if (current_port < 0) {
- return -1;
+ return -2;
}
}
continue;
@@ -357,11 +379,20 @@ int config_read(char *name, struct config *cfg)
switch(current_section) {
- scan_global_line(line, cfg);
+ if (!scan_global_line(line, cfg)) {
+ fprintf(stderr, "syntax error in global section
on line %d\n", line_num);
+ return -2;
+ }
break;
- scan_port_line(line, cfg, current_port);
+ if (!scan_port_line(line, cfg, current_port)) {
+ fprintf(stderr, "syntax error in port section
on line %d\n", line_num);
+ return -2;
+ }

Print out the port section title here. "syntax error in port %s section on line %d\n" for example?

break;
+ fprintf(stderr, "line %d not in section\n", line_num);
+ return -2;

I'd reword this with "line %d is not in a section" maybe?

continue;
}
@@ -378,6 +409,7 @@ int config_create_interface(char *name, struct config *cfg)
int i;
if (cfg->nports >= MAX_PORTS) {
+ fprintf(stderr, "more than %d ports specified\n", MAX_PORTS);
return -1;
}
diff --git a/ptp4l.c b/ptp4l.c
index bc2bc24..560d6d6 100644
--- a/ptp4l.c
+++ b/ptp4l.c
@@ -229,9 +229,16 @@ int main(int argc, char *argv[])
}
}
- if (config && config_read(config, &cfg_settings)) {
- fprintf(stderr, "failed to read configuration file\n");
- return -1;
+ if (config && (c = config_read(config, &cfg_settings))) {
+ switch (c) {
+ fprintf(stderr, "failed to read configuration
file\n");
+ break;
+ fprintf(stderr, "failed to parse configuration
file\n");
+ break;
+ }
+ return c;
}
if (ds->slaveOnly) {
ds->clockQuality.clockClass = 255;
--
1.7.11.7
--------------------------------------------------------------------------
----
LogMeIn Central: Instant, anywhere, Remote PC access and management.
Stay in control, update software, and manage PCs from one command center
Diagnose problems and improve visibility into emerging IT issues
Automate, monitor and manage. Do more in less time with Central
http://p.sf.net/sfu/logmein12331_d2d
_______________________________________________
Linuxptp-devel mailing list
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel

[Miroslav Lichvar]

Post by Keller, Jacob E
I have a few comments inline. General comments, would it be possible to explicitly print out error messages for the main sections if it knows the option but the value doesn't match?
For example if you set timestamping mode and that value isn't recognized could we print out a message "%s is an unrecognized value for %s option"?

I think it's possible, I'll see what I can do.

Post by Keller, Jacob E

Post by Miroslav Lichvar
@@ -121,6 +121,9 @@ static void scan_port_line(const char *s, struct
config *cfg, int p)
cfg->iface[p].transport = TRANS_UDP_IPV6;
+ else
+ return 0;
+

usually return 0 is for success, non-zero for failure. Any reason you chose otherwise here?

The scan_pod and scan_mode functions return 1 for success. I'll
convert them all to return 0 and fix all the other issues you
mentioned.

Thanks for the review.

[Richard Cochran]

Applied.

Thanks,
Richard