libnetplan: don't try to read from a NULL file#342
Conversation
When trying to build an error context for a file without a name (when
using netplan set for example), the filename will be NULL. We are
triggering some critical assertions from glib thanks to that:
GLib-GIO-CRITICAL ...: g_file_new_for_path: assertion 'path != NULL' failed
In this cases the context will be meaningless:
(null):4:14: Error in network definition: invalid boolean value 'falsea'
(null)
^
Also, add G_DEBUG=fatal_criticals to the test environment so we'll
segfault if we assert on a critical issue again.
slyon
left a comment
slyon
left a comment
There was a problem hiding this comment.
LGTM.
Just one suggestion/idea: What if we would start passing around the file-descriptor of our temp file, starting in netplan_parser_load_yaml_from_fd/_netplan_parser_load_single_file down into yaml_error/get_syntax_error_context. We should then be able to show that this failure originates from a "tempfile", but still show the parser context (line & col). Not all error messages specify the settings-key, so this could be helpful in some cases.
OTOH the content of the tempfile is mostly auto-generated in memory, so the context is not necessarily helpful to the user. WDYT?
I'm fine merging it as-is, too. But we should be aware that this could interfere with #334 where we're trying to parse out the filepath:line:col from the error message, which might not exist anymore in all cases after this got merged.
|
I actually had the same thought about passing the FD around and read the auto generated file. But as you said, the user is not supposed to see that file. Providing context with references to its contents might make things confusing I guess. As for #334, yeah if we merge this one I will need to adjust the PR, but it's still a draft that needs more work anyway... |
|
ACK. So let's move ahead and merge this. |
2 participants
When trying to build an error context for a file without a name (when using netplan set for example), the filename will be NULL. We are triggering some critical assertions from glib thanks to that:
In this cases the context will be meaningless:
Also, add G_DEBUG=fatal_criticals to the test environment so we'll segfault if we assert on a critical issue again.
It will change the error message for the command
netplan set --root-dir /tmp/fakeroot bridges.eth12.dhcp4=falseafrom:to:
Description
Checklist
make checksuccessfully.make check-coverage).