* [PATCH] media-ctl error messages
@ 2013-05-08 13:27 Sascha Hauer
2013-05-08 13:27 ` [PATCH 1/2] Print more detailed parse " Sascha Hauer
2013-05-08 13:27 ` [PATCH 2/2] Print parser position on error Sascha Hauer
0 siblings, 2 replies; 8+ messages in thread
From: Sascha Hauer @ 2013-05-08 13:27 UTC (permalink / raw)
To: Laurent Pinchart; +Cc: linux-media
Hi All,
As a first time media-ctl user it was quite frustrating to see that whatever I
did media-ctl only responded with "Unable to parse link". The following is an
attempt to add some more detailed error messages. With this applied media-ctl
can answer with something like:
No pad '1' on entity "mt9p031 0-0048". Maximum pad number is 0
media_parse_setup_link: Unable to parse link
"mt9p031 0-0048":1->"/soc/cammultiplex@plx0":1[0]
^
Please consider applying.
Sascha
----------------------------------------------------------------
Sascha Hauer (2):
Print more detailed parse error messages
Print parser position on error
src/mediactl.c | 83 +++++++++++++++++++++++++++++++++++++++++++++++++---------
1 file changed, 71 insertions(+), 12 deletions(-)
^ permalink raw reply [flat|nested] 8+ messages in thread* [PATCH 1/2] Print more detailed parse error messages 2013-05-08 13:27 [PATCH] media-ctl error messages Sascha Hauer @ 2013-05-08 13:27 ` Sascha Hauer 2013-05-08 13:32 ` Sascha Hauer 2013-05-08 13:27 ` [PATCH 2/2] Print parser position on error Sascha Hauer 1 sibling, 1 reply; 8+ messages in thread From: Sascha Hauer @ 2013-05-08 13:27 UTC (permalink / raw) To: Laurent Pinchart; +Cc: linux-media, Sascha Hauer The following errors usually resulted in the same 'Unable to parse link' message: - one of the given entities does not exist - one of the pads of a given entity does not exist - No link exists between given pads - syntax error in link description Add more detailed error messages to give the user a clue what is going wrong. Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> --- src/mediactl.c | 35 ++++++++++++++++++++++++++--------- 1 file changed, 26 insertions(+), 9 deletions(-) diff --git a/src/mediactl.c b/src/mediactl.c index 4783a58..c65de50 100644 --- a/src/mediactl.c +++ b/src/mediactl.c @@ -537,31 +537,42 @@ struct media_pad *media_parse_pad(struct media_device *media, if (*p == '"') { for (end = (char *)p + 1; *end && *end != '"'; ++end); - if (*end != '"') + if (*end != '"') { + media_dbg(media, "missing matching '\"'\n"); return NULL; + } entity = media_get_entity_by_name(media, p + 1, end - p - 1); - if (entity == NULL) + if (entity == NULL) { + media_dbg(media, "no such entity \"%.*s\"\n", end - p - 1, p + 1); return NULL; + } ++end; } else { entity_id = strtoul(p, &end, 10); entity = media_get_entity_by_id(media, entity_id); - if (entity == NULL) + if (entity == NULL) { + media_dbg(media, "no such entity %d\n", entity_id); return NULL; + } } for (; isspace(*end); ++end); - if (*end != ':') + if (*end != ':') { + media_dbg(media, "Expected ':'\n", *end); return NULL; + } + for (p = end + 1; isspace(*p); ++p); pad = strtoul(p, &end, 10); - for (p = end; isspace(*p); ++p); - if (pad >= entity->info.pads) + if (pad >= entity->info.pads) { + media_dbg(media, "No pad '%d' on entity \"%s\". Maximum pad number is %d\n", + pad, entity->info.name, entity->info.pads - 1); return NULL; + } for (p = end; isspace(*p); ++p); if (endp) @@ -583,8 +594,11 @@ struct media_link *media_parse_link(struct media_device *media, if (source == NULL) return NULL; - if (end[0] != '-' || end[1] != '>') + if (end[0] != '-' || end[1] != '>') { + media_dbg(media, "Expected '->'\n"); return NULL; + } + p = end + 2; sink = media_parse_pad(media, p, &end); @@ -600,6 +614,9 @@ struct media_link *media_parse_link(struct media_device *media, return link; } + media_dbg(media, "No link between \"%s\":%d and \"%s\":%d\n", + source->entity->info.name, source->index, + sink->entity->info.name, sink->index); return NULL; } @@ -619,14 +636,14 @@ int media_parse_setup_link(struct media_device *media, p = end; if (*p++ != '[') { - media_dbg(media, "Unable to parse link flags\n"); + media_dbg(media, "Unable to parse link flags: expected '['.\n"); return -EINVAL; } flags = strtoul(p, &end, 10); for (p = end; isspace(*p); p++); if (*p++ != ']') { - media_dbg(media, "Unable to parse link flags\n"); + media_dbg(media, "Unable to parse link flags: expected ']'.\n"); return -EINVAL; } -- 1.8.2.rc2 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] Print more detailed parse error messages 2013-05-08 13:27 ` [PATCH 1/2] Print more detailed parse " Sascha Hauer @ 2013-05-08 13:32 ` Sascha Hauer 0 siblings, 0 replies; 8+ messages in thread From: Sascha Hauer @ 2013-05-08 13:32 UTC (permalink / raw) To: Laurent Pinchart; +Cc: linux-media On Wed, May 08, 2013 at 03:27:53PM +0200, Sascha Hauer wrote: > The following errors usually resulted in the same 'Unable to parse link' > message: > > - one of the given entities does not exist > - one of the pads of a given entity does not exist > - No link exists between given pads > - syntax error in link description > > Add more detailed error messages to give the user a clue what is going wrong. > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> > --- > src/mediactl.c | 35 ++++++++++++++++++++++++++--------- > 1 file changed, 26 insertions(+), 9 deletions(-) > > diff --git a/src/mediactl.c b/src/mediactl.c > index 4783a58..c65de50 100644 > --- a/src/mediactl.c > +++ b/src/mediactl.c > @@ -537,31 +537,42 @@ struct media_pad *media_parse_pad(struct media_device *media, > > if (*p == '"') { > for (end = (char *)p + 1; *end && *end != '"'; ++end); > - if (*end != '"') > + if (*end != '"') { > + media_dbg(media, "missing matching '\"'\n"); > return NULL; > + } > > entity = media_get_entity_by_name(media, p + 1, end - p - 1); > - if (entity == NULL) > + if (entity == NULL) { > + media_dbg(media, "no such entity \"%.*s\"\n", end - p - 1, p + 1); > return NULL; > + } > > ++end; > } else { > entity_id = strtoul(p, &end, 10); > entity = media_get_entity_by_id(media, entity_id); > - if (entity == NULL) > + if (entity == NULL) { > + media_dbg(media, "no such entity %d\n", entity_id); > return NULL; > + } > } > for (; isspace(*end); ++end); > > - if (*end != ':') > + if (*end != ':') { > + media_dbg(media, "Expected ':'\n", *end); > return NULL; > + } > + > for (p = end + 1; isspace(*p); ++p); > > pad = strtoul(p, &end, 10); > - for (p = end; isspace(*p); ++p); Oops, this maybe should be a separate patch. This is not needed here... > > - if (pad >= entity->info.pads) > + if (pad >= entity->info.pads) { > + media_dbg(media, "No pad '%d' on entity \"%s\". Maximum pad number is %d\n", > + pad, entity->info.name, entity->info.pads - 1); > return NULL; > + } > > for (p = end; isspace(*p); ++p); ... since eating whitespaces once is enough. Sascha -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 2/2] Print parser position on error 2013-05-08 13:27 [PATCH] media-ctl error messages Sascha Hauer 2013-05-08 13:27 ` [PATCH 1/2] Print more detailed parse " Sascha Hauer @ 2013-05-08 13:27 ` Sascha Hauer 2013-06-10 13:56 ` Laurent Pinchart 1 sibling, 1 reply; 8+ messages in thread From: Sascha Hauer @ 2013-05-08 13:27 UTC (permalink / raw) To: Laurent Pinchart; +Cc: linux-media, Sascha Hauer Most parser functions take a **endp argument to indicate the caller where parsing has stopped. This is currently only used after parsing something successfully. This patch sets **endp to the erroneous position in the error case and prints its position after an error has occured. Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> --- src/mediactl.c | 48 +++++++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 45 insertions(+), 3 deletions(-) diff --git a/src/mediactl.c b/src/mediactl.c index c65de50..04ade15 100644 --- a/src/mediactl.c +++ b/src/mediactl.c @@ -40,6 +40,22 @@ #include "mediactl.h" #include "tools.h" +void media_print_streampos(struct media_device *media, const char *p, const char *end) +{ + int pos; + + pos = end - p + 1; + + if (pos < 0) + pos = 0; + if (pos > strlen(p)) + pos = strlen(p); + + media_dbg(media, "\n"); + media_dbg(media, " %s\n", p); + media_dbg(media, " %*s\n", pos, "^"); +} + struct media_pad *media_entity_remote_source(struct media_pad *pad) { unsigned int i; @@ -538,12 +554,16 @@ struct media_pad *media_parse_pad(struct media_device *media, if (*p == '"') { for (end = (char *)p + 1; *end && *end != '"'; ++end); if (*end != '"') { + if (endp) + *endp = (char *)end; media_dbg(media, "missing matching '\"'\n"); return NULL; } entity = media_get_entity_by_name(media, p + 1, end - p - 1); if (entity == NULL) { + if (endp) + *endp = (char *)p + 1; media_dbg(media, "no such entity \"%.*s\"\n", end - p - 1, p + 1); return NULL; } @@ -553,6 +573,8 @@ struct media_pad *media_parse_pad(struct media_device *media, entity_id = strtoul(p, &end, 10); entity = media_get_entity_by_id(media, entity_id); if (entity == NULL) { + if (endp) + *endp = (char *)p; media_dbg(media, "no such entity %d\n", entity_id); return NULL; } @@ -560,6 +582,8 @@ struct media_pad *media_parse_pad(struct media_device *media, for (; isspace(*end); ++end); if (*end != ':') { + if (endp) + *endp = end; media_dbg(media, "Expected ':'\n", *end); return NULL; } @@ -569,6 +593,8 @@ struct media_pad *media_parse_pad(struct media_device *media, pad = strtoul(p, &end, 10); if (pad >= entity->info.pads) { + if (endp) + *endp = (char *)p; media_dbg(media, "No pad '%d' on entity \"%s\". Maximum pad number is %d\n", pad, entity->info.name, entity->info.pads - 1); return NULL; @@ -591,10 +617,15 @@ struct media_link *media_parse_link(struct media_device *media, char *end; source = media_parse_pad(media, p, &end); - if (source == NULL) + if (source == NULL) { + if (endp) + *endp = end; return NULL; + } if (end[0] != '-' || end[1] != '>') { + if (endp) + *endp = end; media_dbg(media, "Expected '->'\n"); return NULL; } @@ -602,8 +633,11 @@ struct media_link *media_parse_link(struct media_device *media, p = end + 2; sink = media_parse_pad(media, p, &end); - if (sink == NULL) + if (sink == NULL) { + if (endp) + *endp = end; return NULL; + } *endp = end; @@ -629,6 +663,8 @@ int media_parse_setup_link(struct media_device *media, link = media_parse_link(media, p, &end); if (link == NULL) { + if (endp) + *endp = end; media_dbg(media, "%s: Unable to parse link\n", __func__); return -EINVAL; @@ -636,6 +672,8 @@ int media_parse_setup_link(struct media_device *media, p = end; if (*p++ != '[') { + if (endp) + *endp = (char *)p - 1; media_dbg(media, "Unable to parse link flags: expected '['.\n"); return -EINVAL; } @@ -643,6 +681,8 @@ int media_parse_setup_link(struct media_device *media, flags = strtoul(p, &end, 10); for (p = end; isspace(*p); p++); if (*p++ != ']') { + if (endp) + *endp = (char *)p - 1; media_dbg(media, "Unable to parse link flags: expected ']'.\n"); return -EINVAL; } @@ -666,8 +706,10 @@ int media_parse_setup_links(struct media_device *media, const char *p) do { ret = media_parse_setup_link(media, p, &end); - if (ret < 0) + if (ret < 0) { + media_print_streampos(media, p, end); return ret; + } p = end + 1; } while (*end == ','); -- 1.8.2.rc2 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] Print parser position on error 2013-05-08 13:27 ` [PATCH 2/2] Print parser position on error Sascha Hauer @ 2013-06-10 13:56 ` Laurent Pinchart 2013-06-19 0:39 ` Laurent Pinchart 0 siblings, 1 reply; 8+ messages in thread From: Laurent Pinchart @ 2013-06-10 13:56 UTC (permalink / raw) To: Sascha Hauer; +Cc: linux-media Hi Sascha, I'm sorry for the late reply. Great patch set, thank you. It's very helpful. On Wednesday 08 May 2013 15:27:54 Sascha Hauer wrote: > Most parser functions take a **endp argument to indicate the caller > where parsing has stopped. This is currently only used after parsing > something successfully. This patch sets **endp to the erroneous > position in the error case and prints its position after an error > has occured. > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> > --- > src/mediactl.c | 48 +++++++++++++++++++++++++++++++++++++++++++++--- > 1 file changed, 45 insertions(+), 3 deletions(-) > > diff --git a/src/mediactl.c b/src/mediactl.c > index c65de50..04ade15 100644 > --- a/src/mediactl.c > +++ b/src/mediactl.c > @@ -40,6 +40,22 @@ > #include "mediactl.h" > #include "tools.h" > > +void media_print_streampos(struct media_device *media, const char *p, const > char *end) The function can be static. > +{ > + int pos; > + > + pos = end - p + 1; > + > + if (pos < 0) > + pos = 0; > + if (pos > strlen(p)) > + pos = strlen(p); > + > + media_dbg(media, "\n"); > + media_dbg(media, " %s\n", p); > + media_dbg(media, " %*s\n", pos, "^"); > +} > + > struct media_pad *media_entity_remote_source(struct media_pad *pad) > { > unsigned int i; > @@ -538,12 +554,16 @@ struct media_pad *media_parse_pad(struct media_device > *media, if (*p == '"') { > for (end = (char *)p + 1; *end && *end != '"'; ++end); > if (*end != '"') { > + if (endp) > + *endp = (char *)end; No need to cast to a char * here, end is already a char *. What would you think about adding + /* endp can be NULL. To avoid spreading NULL checks across the + * function, set endp to &end in that case. + */ + if (endp == NULL) + endp = &end; at the beginning of the function and removing the endp NULL checks that are spread across the code below ? > media_dbg(media, "missing matching '\"'\n"); > return NULL; > } > > entity = media_get_entity_by_name(media, p + 1, end - p - 1); > if (entity == NULL) { > + if (endp) > + *endp = (char *)p + 1; > media_dbg(media, "no such entity \"%.*s\"\n", end - p - 1, p + 1); > return NULL; > } > @@ -553,6 +573,8 @@ struct media_pad *media_parse_pad(struct media_device > *media, entity_id = strtoul(p, &end, 10); > entity = media_get_entity_by_id(media, entity_id); > if (entity == NULL) { > + if (endp) > + *endp = (char *)p; > media_dbg(media, "no such entity %d\n", entity_id); > return NULL; > } > @@ -560,6 +582,8 @@ struct media_pad *media_parse_pad(struct media_device > *media, for (; isspace(*end); ++end); > > if (*end != ':') { > + if (endp) > + *endp = end; > media_dbg(media, "Expected ':'\n", *end); > return NULL; > } > @@ -569,6 +593,8 @@ struct media_pad *media_parse_pad(struct media_device > *media, pad = strtoul(p, &end, 10); > > if (pad >= entity->info.pads) { > + if (endp) > + *endp = (char *)p; > media_dbg(media, "No pad '%d' on entity \"%s\". Maximum pad number is > %d\n", pad, entity->info.name, entity->info.pads - 1); > return NULL; > @@ -591,10 +617,15 @@ struct media_link *media_parse_link(struct > media_device *media, char *end; > > source = media_parse_pad(media, p, &end); > - if (source == NULL) > + if (source == NULL) { > + if (endp) > + *endp = end; The endp argument to media_parse_link() and media_parse_setup_link() is mandatory, there's no need to test it. If you're fine with those modifications there's no need to resubmit the code, I'll fix it while applying. > return NULL; > + } > > if (end[0] != '-' || end[1] != '>') { > + if (endp) > + *endp = end; > media_dbg(media, "Expected '->'\n"); > return NULL; > } > @@ -602,8 +633,11 @@ struct media_link *media_parse_link(struct media_device > *media, p = end + 2; > > sink = media_parse_pad(media, p, &end); > - if (sink == NULL) > + if (sink == NULL) { > + if (endp) > + *endp = end; > return NULL; > + } > > *endp = end; > > @@ -629,6 +663,8 @@ int media_parse_setup_link(struct media_device *media, > > link = media_parse_link(media, p, &end); > if (link == NULL) { > + if (endp) > + *endp = end; > media_dbg(media, > "%s: Unable to parse link\n", __func__); > return -EINVAL; > @@ -636,6 +672,8 @@ int media_parse_setup_link(struct media_device *media, > > p = end; > if (*p++ != '[') { > + if (endp) > + *endp = (char *)p - 1; > media_dbg(media, "Unable to parse link flags: expected '['.\n"); > return -EINVAL; > } > @@ -643,6 +681,8 @@ int media_parse_setup_link(struct media_device *media, > flags = strtoul(p, &end, 10); > for (p = end; isspace(*p); p++); > if (*p++ != ']') { > + if (endp) > + *endp = (char *)p - 1; > media_dbg(media, "Unable to parse link flags: expected ']'.\n"); > return -EINVAL; > } > @@ -666,8 +706,10 @@ int media_parse_setup_links(struct media_device *media, > const char *p) > > do { > ret = media_parse_setup_link(media, p, &end); > - if (ret < 0) > + if (ret < 0) { > + media_print_streampos(media, p, end); > return ret; > + } > > p = end + 1; > } while (*end == ','); -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] Print parser position on error 2013-06-10 13:56 ` Laurent Pinchart @ 2013-06-19 0:39 ` Laurent Pinchart 2013-06-19 5:30 ` Sascha Hauer 0 siblings, 1 reply; 8+ messages in thread From: Laurent Pinchart @ 2013-06-19 0:39 UTC (permalink / raw) To: Sascha Hauer; +Cc: linux-media Hi Sascha, On Monday 10 June 2013 15:56:01 Laurent Pinchart wrote: > Hi Sascha, > > I'm sorry for the late reply. > > Great patch set, thank you. It's very helpful. Should I got ahead and apply the patch with the proposed modifications below ? > On Wednesday 08 May 2013 15:27:54 Sascha Hauer wrote: > > Most parser functions take a **endp argument to indicate the caller > > where parsing has stopped. This is currently only used after parsing > > something successfully. This patch sets **endp to the erroneous > > position in the error case and prints its position after an error > > has occured. > > > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> > > --- > > > > src/mediactl.c | 48 +++++++++++++++++++++++++++++++++++++++++++++--- > > 1 file changed, 45 insertions(+), 3 deletions(-) > > > > diff --git a/src/mediactl.c b/src/mediactl.c > > index c65de50..04ade15 100644 > > --- a/src/mediactl.c > > +++ b/src/mediactl.c > > @@ -40,6 +40,22 @@ > > > > #include "mediactl.h" > > #include "tools.h" > > > > +void media_print_streampos(struct media_device *media, const char *p, > > const char *end) > > The function can be static. > > > +{ > > + int pos; > > + > > + pos = end - p + 1; > > + > > + if (pos < 0) > > + pos = 0; > > + if (pos > strlen(p)) > > + pos = strlen(p); > > + > > + media_dbg(media, "\n"); > > + media_dbg(media, " %s\n", p); > > + media_dbg(media, " %*s\n", pos, "^"); > > +} > > + > > > > struct media_pad *media_entity_remote_source(struct media_pad *pad) > > { > > > > unsigned int i; > > > > @@ -538,12 +554,16 @@ struct media_pad *media_parse_pad(struct > > media_device > > *media, if (*p == '"') { > > > > for (end = (char *)p + 1; *end && *end != '"'; ++end); > > if (*end != '"') { > > > > + if (endp) > > + *endp = (char *)end; > > No need to cast to a char * here, end is already a char *. > > What would you think about adding > > + /* endp can be NULL. To avoid spreading NULL checks across the > + * function, set endp to &end in that case. > + */ > + if (endp == NULL) > + endp = &end; > > at the beginning of the function and removing the endp NULL checks that are > spread across the code below ? > > > media_dbg(media, "missing matching '\"'\n"); > > return NULL; > > > > } > > > > entity = media_get_entity_by_name(media, p + 1, end - p - 1); > > if (entity == NULL) { > > > > + if (endp) > > + *endp = (char *)p + 1; > > > > media_dbg(media, "no such entity \"%.*s\"\n", end - p - 1, p + > > 1); > > > return NULL; > > > > } > > > > @@ -553,6 +573,8 @@ struct media_pad *media_parse_pad(struct media_device > > *media, entity_id = strtoul(p, &end, 10); > > > > entity = media_get_entity_by_id(media, entity_id); > > if (entity == NULL) { > > > > + if (endp) > > + *endp = (char *)p; > > > > media_dbg(media, "no such entity %d\n", entity_id); > > return NULL; > > > > } > > > > @@ -560,6 +582,8 @@ struct media_pad *media_parse_pad(struct media_device > > *media, for (; isspace(*end); ++end); > > > > if (*end != ':') { > > > > + if (endp) > > + *endp = end; > > > > media_dbg(media, "Expected ':'\n", *end); > > return NULL; > > > > } > > > > @@ -569,6 +593,8 @@ struct media_pad *media_parse_pad(struct media_device > > *media, pad = strtoul(p, &end, 10); > > > > if (pad >= entity->info.pads) { > > > > + if (endp) > > + *endp = (char *)p; > > > > media_dbg(media, "No pad '%d' on entity \"%s\". Maximum pad number is > > > > %d\n", pad, entity->info.name, entity->info.pads - 1); > > > > return NULL; > > > > @@ -591,10 +617,15 @@ struct media_link *media_parse_link(struct > > media_device *media, char *end; > > > > source = media_parse_pad(media, p, &end); > > > > - if (source == NULL) > > + if (source == NULL) { > > + if (endp) > > + *endp = end; > > The endp argument to media_parse_link() and media_parse_setup_link() is > mandatory, there's no need to test it. > > If you're fine with those modifications there's no need to resubmit the > code, I'll fix it while applying. > > > return NULL; > > > > + } > > > > if (end[0] != '-' || end[1] != '>') { > > > > + if (endp) > > + *endp = end; > > > > media_dbg(media, "Expected '->'\n"); > > return NULL; > > > > } > > > > @@ -602,8 +633,11 @@ struct media_link *media_parse_link(struct > > media_device *media, p = end + 2; > > > > sink = media_parse_pad(media, p, &end); > > > > - if (sink == NULL) > > + if (sink == NULL) { > > + if (endp) > > + *endp = end; > > > > return NULL; > > > > + } > > > > *endp = end; > > > > @@ -629,6 +663,8 @@ int media_parse_setup_link(struct media_device *media, > > > > link = media_parse_link(media, p, &end); > > if (link == NULL) { > > > > + if (endp) > > + *endp = end; > > > > media_dbg(media, > > > > "%s: Unable to parse link\n", __func__); > > > > return -EINVAL; > > > > @@ -636,6 +672,8 @@ int media_parse_setup_link(struct media_device *media, > > > > p = end; > > if (*p++ != '[') { > > > > + if (endp) > > + *endp = (char *)p - 1; > > > > media_dbg(media, "Unable to parse link flags: expected '['.\n"); > > return -EINVAL; > > > > } > > > > @@ -643,6 +681,8 @@ int media_parse_setup_link(struct media_device *media, > > > > flags = strtoul(p, &end, 10); > > for (p = end; isspace(*p); p++); > > if (*p++ != ']') { > > > > + if (endp) > > + *endp = (char *)p - 1; > > > > media_dbg(media, "Unable to parse link flags: expected ']'.\n"); > > return -EINVAL; > > > > } > > > > @@ -666,8 +706,10 @@ int media_parse_setup_links(struct media_device > > *media, const char *p) > > > > do { > > > > ret = media_parse_setup_link(media, p, &end); > > > > - if (ret < 0) > > + if (ret < 0) { > > + media_print_streampos(media, p, end); > > > > return ret; > > > > + } > > > > p = end + 1; > > > > } while (*end == ','); -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] Print parser position on error 2013-06-19 0:39 ` Laurent Pinchart @ 2013-06-19 5:30 ` Sascha Hauer 2013-06-19 10:40 ` Laurent Pinchart 0 siblings, 1 reply; 8+ messages in thread From: Sascha Hauer @ 2013-06-19 5:30 UTC (permalink / raw) To: Laurent Pinchart; +Cc: linux-media Hi Laurent, On Wed, Jun 19, 2013 at 02:39:48AM +0200, Laurent Pinchart wrote: > Hi Sascha, > > On Monday 10 June 2013 15:56:01 Laurent Pinchart wrote: > > Hi Sascha, > > > > I'm sorry for the late reply. > > > > Great patch set, thank you. It's very helpful. > > Should I got ahead and apply the patch with the proposed modifications below ? That'd be nice. I'm a bit out of the loop at the moment and don't have a setup handy to test the changes. > > No need to cast to a char * here, end is already a char *. > > > > What would you think about adding > > > > + /* endp can be NULL. To avoid spreading NULL checks across the > > + * function, set endp to &end in that case. > > + */ > > + if (endp == NULL) > > + endp = &end; > > > > at the beginning of the function and removing the endp NULL checks that are > > spread across the code below ? Good idea. Your other suggestions also look good. Regards Sascha -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] Print parser position on error 2013-06-19 5:30 ` Sascha Hauer @ 2013-06-19 10:40 ` Laurent Pinchart 0 siblings, 0 replies; 8+ messages in thread From: Laurent Pinchart @ 2013-06-19 10:40 UTC (permalink / raw) To: Sascha Hauer; +Cc: linux-media Hi Sascha, On Wednesday 19 June 2013 07:30:54 Sascha Hauer wrote: > On Wed, Jun 19, 2013 at 02:39:48AM +0200, Laurent Pinchart wrote: > > On Monday 10 June 2013 15:56:01 Laurent Pinchart wrote: > > > Hi Sascha, > > > > > > I'm sorry for the late reply. > > > > > > Great patch set, thank you. It's very helpful. > > > > Should I got ahead and apply the patch with the proposed modifications > > below ? > > That'd be nice. I'm a bit out of the loop at the moment and don't have > a setup handy to test the changes. Pushed, thank you. > > > No need to cast to a char * here, end is already a char *. > > > > > > What would you think about adding > > > > > > + /* endp can be NULL. To avoid spreading NULL checks across the > > > + * function, set endp to &end in that case. > > > + */ > > > + if (endp == NULL) > > > + endp = &end; > > > > > > at the beginning of the function and removing the endp NULL checks that > > > are > > > spread across the code below ? > > Good idea. Your other suggestions also look good. -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2013-06-19 10:40 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2013-05-08 13:27 [PATCH] media-ctl error messages Sascha Hauer 2013-05-08 13:27 ` [PATCH 1/2] Print more detailed parse " Sascha Hauer 2013-05-08 13:32 ` Sascha Hauer 2013-05-08 13:27 ` [PATCH 2/2] Print parser position on error Sascha Hauer 2013-06-10 13:56 ` Laurent Pinchart 2013-06-19 0:39 ` Laurent Pinchart 2013-06-19 5:30 ` Sascha Hauer 2013-06-19 10:40 ` Laurent Pinchart
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).