parse: don't point to the wrong node on validation#343
Conversation
The function validate_netdef_grammar() checks several properties from
the netdef that are not related to the current context (YAML node). This
leads to error messages like these ones:
foo.yaml:5:7: Error ...: eth0: 'set-name:' requires 'match:' properties
macaddress: c0:f3:7e:35:8e:1b
^
foo.yaml:5:7: Error in ...: wlan0: No access points defined
macaddress: c0:f3:7e:35:8e:1b
^
Using the YAML tree node here doesn't seem to make sense, but removing
it would break the ABI.
|
The change looks sensible to me. Could you please give some more context on how this would break ABI? Our ABI checker CI seems to be happy.. |
|
I mean, if we don't want to use the YAML node we could remove it from the functions parameters (the third one). |
slyon
left a comment
slyon
left a comment
There was a problem hiding this comment.
Oh, you mean it would chance ABI when we drop the yaml_node_t* node argument from the function signature?
I assume the easiest would be to just pass NULL from "parse.c", as is done in "parse-nm.c". But I agree the YAML context/node isn't really needed inside this validation function. So cleaning up the singature is fine with me too. The function is not exported publicly, and all consumers are part of libnetplan, so I don't think it should break anything if we drop that parameter from the signature. If needed we could mute that specific case in abi-compat/suppressions.abignore.
It doesn't make much sense having it since the validation will emit errors not related to the node.
slyon
left a comment
slyon
left a comment
There was a problem hiding this comment.
lgtm!
And the ABI checker seems to be happy, too :)
2 participants
The function validate_netdef_grammar() checks several properties from the netdef that are not related to the current context (YAML node). This leads to error messages like these ones:
Using the YAML tree node here doesn't seem to make sense, but removing it would break the ABI.
Description
Checklist
make checksuccessfully.make check-coverage).