From e8a2ba6787228c19e6d36a853275dd4dff64b0bb Mon Sep 17 00:00:00 2001 From: Martin Thierer Date: Sat, 28 Mar 2020 09:06:45 +0100 Subject: [PATCH 1/3] --channels: Allow dash in name when renaming channels. Arguments to the "--channels" option are checked for the range separator (a dash) first before handling a possible rename. This makes it impossible to rename a channel to anything that includes a dash. So instead check for a rename (equals character) first and handle a possible range later. It makes no sense to rename a range of channels so report an error if there is a dash character in the part preceding the equals sign, except if a channel with this exact name exists. --- parsers.c | 66 +++++++++++++++++++++++++++++++------------------------ 1 file changed, 37 insertions(+), 29 deletions(-) diff --git a/parsers.c b/parsers.c index 2aacefb..dab0ba8 100644 --- a/parsers.c +++ b/parsers.c @@ -71,14 +71,41 @@ GSList *parse_channelstring(struct sr_dev_inst *sdi, const char *channelstring) ret = SR_ERR; break; } - if (strchr(tokens[i], '-')) { + + names = g_strsplit(tokens[i], "=", 2); + if (!names[0] || (names[1] && names[2])) { + /* Need one or two arguments. */ + g_critical("Invalid channel '%s'.", tokens[i]); + g_strfreev(names); + ret = SR_ERR; + break; + } + + /* Check first if a channel with the given name already exists, as it + * might contain a dash from a rename in an earlier option. + */ + ch = find_channel(channels, names[0], TRUE); + if (ch) { + if (names[1]) { + /* Rename channel. */ + sr_dev_channel_name_set(ch, names[1]); + } + channellist = g_slist_append(channellist, ch); + } else if (strchr(names[0], '-')) { /* * A range of channels in the form a-b. This will only work * if the channels are named as numbers -- so every channel * in the range must exist as a channel name string in the * device. */ - range = g_strsplit(tokens[i], "-", 2); + if (names[1]) { + /* Channel range syntax not allowed when renaming. */ + g_critical("Invalid rename for channel range '%s'.", names[0]); + ret = SR_ERR; + goto range_fail; + } + + range = g_strsplit(names[0], "-", 2); if (!range[0] || !range[1] || range[2]) { /* Need exactly two arguments. */ g_critical("Invalid channel syntax '%s'.", tokens[i]); @@ -99,7 +126,7 @@ GSList *parse_channelstring(struct sr_dev_inst *sdi, const char *channelstring) goto range_fail; } if (b < 0 || b >= e) { - g_critical("Invalid channel range '%s'.", tokens[i]); + g_critical("Invalid channel range '%s'.", names[0]); ret = SR_ERR; goto range_fail; } @@ -123,34 +150,15 @@ GSList *parse_channelstring(struct sr_dev_inst *sdi, const char *channelstring) range_fail: if (range) g_strfreev(range); + } else { /* Channel name not found and not a channel range. */ + g_critical("unknown channel '%s'.", names[0]); + ret = SR_ERR; + } - if (ret != SR_OK) - break; - } else { - names = g_strsplit(tokens[i], "=", 2); - if (!names[0] || (names[1] && names[2])) { - /* Need one or two arguments. */ - g_critical("Invalid channel '%s'.", tokens[i]); - g_strfreev(names); - ret = SR_ERR; - break; - } - - ch = find_channel(channels, names[0], TRUE); - if (!ch) { - g_critical("unknown channel '%s'.", names[0]); - g_strfreev(names); - ret = SR_ERR; - break; - } - if (names[1]) { - /* Rename channel. */ - sr_dev_channel_name_set(ch, names[1]); - } - channellist = g_slist_append(channellist, ch); + g_strfreev(names); - g_strfreev(names); - } + if (ret != SR_OK) + break; } if (ret != SR_OK) { From b158f42404e6119d81013273c9bdf328772b1c1e Mon Sep 17 00:00:00 2001 From: Martin Thierer Date: Sat, 28 Mar 2020 09:48:48 +0100 Subject: [PATCH 2/3] --channels: Allow common prefix in channel ranges. Selecting a range of channels only works if the channel names are purely numeric, but not if they have a common, non-numeric prefix (eg as in "D0", "D1", ..). This patch allows to use such channels in ranges by checking for a common prefix. The syntax to for example select channels "D2" through to "D5" is "D2-5". Also adjust the manpage to include the new syntax and fix the example that actually didn't work as the fx2lafw driver uses "D0", "D1", ... as channel names. --- doc/sigrok-cli.1 | 6 ++++-- parsers.c | 34 ++++++++++++++++++++++++---------- 2 files changed, 28 insertions(+), 12 deletions(-) diff --git a/doc/sigrok-cli.1 b/doc/sigrok-cli.1 index 33174cd..7e56032 100644 --- a/doc/sigrok-cli.1 +++ b/doc/sigrok-cli.1 @@ -201,13 +201,15 @@ The default is to use all the channels available on a device. You can name a channel like this: .BR "1=CLK" . A range of channels can also be given, in the form -.BR "1\-5" . +.BR "1\-5" +or (if they have a common prefix) +.BR "CH1\-5" . .sp Example: .sp .RB " $ " "sigrok\-cli \-\-driver fx2lafw \-\-samples 100" .br -.B " \-\-channels 1=CLK,2\-4,7" +.B " \-\-channels D1=CLK,D2\-4,D7" .br CLK:11111111 11111111 11111111 11111111 [...] 2:11111111 11111111 11111111 11111111 [...] diff --git a/parsers.c b/parsers.c index dab0ba8..b4187f2 100644 --- a/parsers.c +++ b/parsers.c @@ -18,6 +18,7 @@ */ #include +#include #include #include #include @@ -51,8 +52,8 @@ GSList *parse_channelstring(struct sr_dev_inst *sdi, const char *channelstring) { struct sr_channel *ch; GSList *channellist, *channels; - int ret, n, b, e, i; - char **tokens, **range, **names, *eptr, str[8]; + int ret, n, b, e, i, plen; + char **tokens, **range, **names, *sptr, *eptr, str[8]; channels = sr_dev_inst_channels_get(sdi); @@ -94,9 +95,9 @@ GSList *parse_channelstring(struct sr_dev_inst *sdi, const char *channelstring) } else if (strchr(names[0], '-')) { /* * A range of channels in the form a-b. This will only work - * if the channels are named as numbers -- so every channel - * in the range must exist as a channel name string in the - * device. + * if the channels are named as numbers (with an optional prefix) + * -- so every channel in the range must exist as a channel name + * string in the device. */ if (names[1]) { /* Channel range syntax not allowed when renaming. */ @@ -113,15 +114,28 @@ GSList *parse_channelstring(struct sr_dev_inst *sdi, const char *channelstring) goto range_fail; } + /* Find start of numeric range (first digit) in range string. */ + for (sptr = names[0]; *sptr != '-' && !isdigit(*sptr); sptr++); + if (*sptr == '-') { /* no digit found before range separator. */ + g_critical("Channel range '%s' needs numbered channels.", names[0]); + ret = SR_ERR; + goto range_fail; + } + + /* Length of channel name prefix. */ + plen = sptr - names[0]; + + range = g_strsplit(sptr, "-", 2); + b = strtol(range[0], &eptr, 10); if (eptr == range[0] || *eptr != '\0') { - g_critical("Invalid channel '%s'.", range[0]); + g_critical("Invalid channel '%.*s%s'.", plen, names[0], range[0]); ret = SR_ERR; goto range_fail; } e = strtol(range[1], NULL, 10); if (eptr == range[1] || *eptr != '\0') { - g_critical("Invalid channel '%s'.", range[1]); + g_critical("Invalid channel '%.*s%s'.", plen, names[0], range[1]); ret = SR_ERR; goto range_fail; } @@ -132,15 +146,15 @@ GSList *parse_channelstring(struct sr_dev_inst *sdi, const char *channelstring) } while (b <= e) { - n = snprintf(str, 8, "%d", b); + n = snprintf(str, 8, "%.*s%d", plen, names[0], b); if (n < 0 || n > 8) { - g_critical("Invalid channel '%d'.", b); + g_critical("Invalid channel '%.*s%d'.", plen, names[0], b); ret = SR_ERR; break; } ch = find_channel(channels, str, TRUE); if (!ch) { - g_critical("unknown channel '%d'.", b); + g_critical("unknown channel '%s'.", str); ret = SR_ERR; break; } From d0d27540eae04a15742d576cde6868e6eccd903c Mon Sep 17 00:00:00 2001 From: Martin Thierer Date: Sat, 28 Mar 2020 14:29:49 +0100 Subject: [PATCH 3/3] parse_channelstring(): remove useless checks of g_strsplit() results. 1. !names[0] could only be true if tokens[i] is an empty string, but we already checked at the beginning of the for() loop that this is not the case. "(names[1] && names[2])" can also never be true, as g_strsplit(x, 2) returns a NULL-terminated pointer array with either one or a maximum of two non-NULL strings, so either names[1] or names[2] is always == NULL. 2. Likewise, !range[0] could only be true if names[0] is an empty string. But we already asserted that it contains at least a dash character. So both range[0] and range[1] at least contain pointers to empty strings. g_strsplit(x, 2) returns a NULL-terminated pointer array with a maximum of (and in this case, exactly) two non-NULL strings, which means range[2] is always == NULL. As a result, "!range[0] || !range[1] || range[2]" can never be true. --- parsers.c | 13 ------------- 1 file changed, 13 deletions(-) diff --git a/parsers.c b/parsers.c index b4187f2..edb0244 100644 --- a/parsers.c +++ b/parsers.c @@ -74,13 +74,6 @@ GSList *parse_channelstring(struct sr_dev_inst *sdi, const char *channelstring) } names = g_strsplit(tokens[i], "=", 2); - if (!names[0] || (names[1] && names[2])) { - /* Need one or two arguments. */ - g_critical("Invalid channel '%s'.", tokens[i]); - g_strfreev(names); - ret = SR_ERR; - break; - } /* Check first if a channel with the given name already exists, as it * might contain a dash from a rename in an earlier option. @@ -107,12 +100,6 @@ GSList *parse_channelstring(struct sr_dev_inst *sdi, const char *channelstring) } range = g_strsplit(names[0], "-", 2); - if (!range[0] || !range[1] || range[2]) { - /* Need exactly two arguments. */ - g_critical("Invalid channel syntax '%s'.", tokens[i]); - ret = SR_ERR; - goto range_fail; - } /* Find start of numeric range (first digit) in range string. */ for (sptr = names[0]; *sptr != '-' && !isdigit(*sptr); sptr++);