[PATCH] media-ctl error messages

[PATCH] media-ctl error messages

[Sascha Hauer]

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(-)

[PATCH 1/2] Print more detailed parse error messages

[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

[PATCH 2/2] Print parser position on error

[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

Re: [PATCH 1/2] Print more detailed parse error messages

[Sascha Hauer]

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 |

Re: [PATCH 2/2] Print parser position on error

[Laurent Pinchart]

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

Re: [PATCH 2/2] Print parser position on error

[Laurent Pinchart]

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

Re: [PATCH 2/2] Print parser position on error

[Sascha Hauer]

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 |

Re: [PATCH 2/2] Print parser position on error

[Laurent Pinchart]

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