Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

libnetplan: don't try to read from a NULL file #342

Merged
merged 1 commit into from
Apr 24, 2023

Conversation

daniloegea
Copy link
Contributor

@daniloegea daniloegea commented Apr 20, 2023

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.

It will change the error message for the command netplan set --root-dir /tmp/fakeroot bridges.eth12.dhcp4=falsea from:

netplan.libnetplan.LibNetplanException: (null):4:14: Error in network definition: invalid boolean value 'falsea'
(null)
             ^

to:

netplan.libnetplan.LibNetplanException: Error in network definition: invalid boolean value 'falsea'

Description

Checklist

  • Runs make check successfully.
  • Retains 100% code coverage (make check-coverage).
  • New/changed keys in YAML format are documented.
  • (Optional) Adds example YAML for new feature.
  • (Optional) Closes an open bug in Launchpad.

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.
@daniloegea daniloegea requested a review from slyon April 20, 2023 10:50
Copy link
Collaborator

@slyon slyon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@daniloegea
Copy link
Contributor Author

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...

@slyon
Copy link
Collaborator

slyon commented Apr 24, 2023

ACK. So let's move ahead and merge this.

@slyon slyon merged commit a1a0bd6 into canonical:main Apr 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants