libcamera: yaml_parser: Output more details when parsing fails
diff mbox series

Message ID 20241204111017.343760-1-stefan.klug@ideasonboard.com
State Accepted
Commit fa0013c953b488ae421e66c0de726bf351072589
Headers show
Series
  • libcamera: yaml_parser: Output more details when parsing fails
Related show

Commit Message

Stefan Klug Dec. 4, 2024, 11:10 a.m. UTC 
On malformed yaml files, the yaml parser only errors out without giving
details on the error that happened. Fix that by providing a more detailed
error message.

Output old:

ERROR YamlParser yaml_parser.cpp:886 Failed to parse YAML content from /root/imx283.yaml

Output new:

ERROR YamlParser yaml_parser.cpp:627 /root/imx283.yaml:72:8 could not find expected ':' while scanning a simple key
ERROR YamlParser yaml_parser.cpp:886 Failed to parse YAML content from /root/imx283.yaml

Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
---
 src/libcamera/yaml_parser.cpp | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

Comments

Kieran Bingham Dec. 4, 2024, 11:34 a.m. UTC | #1 
Quoting Stefan Klug (2024-12-04 11:10:13)
> On malformed yaml files, the yaml parser only errors out without giving
> details on the error that happened. Fix that by providing a more detailed
> error message.
> 
> Output old:
> 
> ERROR YamlParser yaml_parser.cpp:886 Failed to parse YAML content from /root/imx283.yaml
> 
> Output new:
> 
> ERROR YamlParser yaml_parser.cpp:627 /root/imx283.yaml:72:8 could not find expected ':' while scanning a simple key

Well, I can't say no to that!

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

> ERROR YamlParser yaml_parser.cpp:886 Failed to parse YAML content from /root/imx283.yaml
> 
> Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> ---
>  src/libcamera/yaml_parser.cpp | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/src/libcamera/yaml_parser.cpp b/src/libcamera/yaml_parser.cpp
> index db256ec5b04d..f9302c4ee3fa 100644
> --- a/src/libcamera/yaml_parser.cpp
> +++ b/src/libcamera/yaml_parser.cpp
> @@ -509,8 +509,17 @@ YamlParserContext::EventPtr YamlParserContext::nextEvent()
>         EventPtr event(new yaml_event_t);
>  
>         /* yaml_parser_parse returns 1 when it succeeds */
> -       if (!yaml_parser_parse(&parser_, event.get()))
> +       if (!yaml_parser_parse(&parser_, event.get())) {
> +               File *file = static_cast<File *>(parser_.read_handler_data);
> +
> +               LOG(YamlParser, Error) << file->fileName() << ":"
> +                                      << parser_.problem_mark.line << ":"
> +                                      << parser_.problem_mark.column << " "
> +                                      << parser_.problem << " "
> +                                      << parser_.context;
> +
>                 return nullptr;
> +       }
>  
>         return event;
>  }
> -- 
> 2.43.0
>
Laurent Pinchart Dec. 5, 2024, 8:12 p.m. UTC | #2 
Hi Stefan,

Thank you for the patch.

On Wed, Dec 04, 2024 at 12:10:13PM +0100, Stefan Klug wrote:
> On malformed yaml files, the yaml parser only errors out without giving
> details on the error that happened. Fix that by providing a more detailed
> error message.
> 
> Output old:
> 
> ERROR YamlParser yaml_parser.cpp:886 Failed to parse YAML content from /root/imx283.yaml
> 
> Output new:
> 
> ERROR YamlParser yaml_parser.cpp:627 /root/imx283.yaml:72:8 could not find expected ':' while scanning a simple key
> ERROR YamlParser yaml_parser.cpp:886 Failed to parse YAML content from /root/imx283.yaml
> 
> Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> ---
>  src/libcamera/yaml_parser.cpp | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/src/libcamera/yaml_parser.cpp b/src/libcamera/yaml_parser.cpp
> index db256ec5b04d..f9302c4ee3fa 100644
> --- a/src/libcamera/yaml_parser.cpp
> +++ b/src/libcamera/yaml_parser.cpp
> @@ -509,8 +509,17 @@ YamlParserContext::EventPtr YamlParserContext::nextEvent()

I wonder what else you have in your tree if the message prints line 627,
as indicated in the example in the commit message :-)

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

>  	EventPtr event(new yaml_event_t);
>  
>  	/* yaml_parser_parse returns 1 when it succeeds */
> -	if (!yaml_parser_parse(&parser_, event.get()))
> +	if (!yaml_parser_parse(&parser_, event.get())) {
> +		File *file = static_cast<File *>(parser_.read_handler_data);
> +
> +		LOG(YamlParser, Error) << file->fileName() << ":"
> +				       << parser_.problem_mark.line << ":"
> +				       << parser_.problem_mark.column << " "
> +				       << parser_.problem << " "
> +				       << parser_.context;
> +
>  		return nullptr;
> +	}
>  
>  	return event;
>  }
Stefan Klug Dec. 6, 2024, 7:04 a.m. UTC | #3 
Hi Laurent,

Thank you for the review. 

On Thu, Dec 05, 2024 at 10:12:43PM +0200, Laurent Pinchart wrote:
> Hi Stefan,
> 
> Thank you for the patch.
> 
> On Wed, Dec 04, 2024 at 12:10:13PM +0100, Stefan Klug wrote:
> > On malformed yaml files, the yaml parser only errors out without giving
> > details on the error that happened. Fix that by providing a more detailed
> > error message.
> > 
> > Output old:
> > 
> > ERROR YamlParser yaml_parser.cpp:886 Failed to parse YAML content from /root/imx283.yaml
> > 
> > Output new:
> > 
> > ERROR YamlParser yaml_parser.cpp:627 /root/imx283.yaml:72:8 could not find expected ':' while scanning a simple key
> > ERROR YamlParser yaml_parser.cpp:886 Failed to parse YAML content from /root/imx283.yaml
> > 
> > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> > ---
> >  src/libcamera/yaml_parser.cpp | 11 ++++++++++-
> >  1 file changed, 10 insertions(+), 1 deletion(-)
> > 
> > diff --git a/src/libcamera/yaml_parser.cpp b/src/libcamera/yaml_parser.cpp
> > index db256ec5b04d..f9302c4ee3fa 100644
> > --- a/src/libcamera/yaml_parser.cpp
> > +++ b/src/libcamera/yaml_parser.cpp
> > @@ -509,8 +509,17 @@ YamlParserContext::EventPtr YamlParserContext::nextEvent()
> 
> I wonder what else you have in your tree if the message prints line 627,
> as indicated in the example in the commit message :-)

Interesting indeed. I checked the logs. The patch was written just
before you merged the "Use std::from_chars()" cleanup that removed quite
some code. I guess that explains the difference.

Regards,
Stefan

> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> >  	EventPtr event(new yaml_event_t);
> >  
> >  	/* yaml_parser_parse returns 1 when it succeeds */
> > -	if (!yaml_parser_parse(&parser_, event.get()))
> > +	if (!yaml_parser_parse(&parser_, event.get())) {
> > +		File *file = static_cast<File *>(parser_.read_handler_data);
> > +
> > +		LOG(YamlParser, Error) << file->fileName() << ":"
> > +				       << parser_.problem_mark.line << ":"
> > +				       << parser_.problem_mark.column << " "
> > +				       << parser_.problem << " "
> > +				       << parser_.context;
> > +
> >  		return nullptr;
> > +	}
> >  
> >  	return event;
> >  }
> 
> -- 
> Regards,
> 
> Laurent Pinchart

Patch
diff mbox series 

diff --git a/src/libcamera/yaml_parser.cpp b/src/libcamera/yaml_parser.cpp
index db256ec5b04d..f9302c4ee3fa 100644
--- a/src/libcamera/yaml_parser.cpp
+++ b/src/libcamera/yaml_parser.cpp
@@ -509,8 +509,17 @@  YamlParserContext::EventPtr YamlParserContext::nextEvent()
 	EventPtr event(new yaml_event_t);
 
 	/* yaml_parser_parse returns 1 when it succeeds */
-	if (!yaml_parser_parse(&parser_, event.get()))
+	if (!yaml_parser_parse(&parser_, event.get())) {
+		File *file = static_cast<File *>(parser_.read_handler_data);
+
+		LOG(YamlParser, Error) << file->fileName() << ":"
+				       << parser_.problem_mark.line << ":"
+				       << parser_.problem_mark.column << " "
+				       << parser_.problem << " "
+				       << parser_.context;
+
 		return nullptr;
+	}
 
 	return event;
 }