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

Fixes for minor issues #380

Merged
merged 12 commits into from Jul 19, 2023
Merged

Fixes for minor issues #380

merged 12 commits into from Jul 19, 2023

Conversation

daniloegea
Copy link
Collaborator

@daniloegea daniloegea commented Jul 17, 2023

Description

Some of this issues were found when I tried to compile netplan as c++, some were found by increasing the warning level with -Wextra and some were found by me while fixing the issues found by the compiler.

Some changes might seem unnecessary, specially the one annotating unused parameters with __unused. But I think they add some value.

My motivation to increase the warning_level to 2 is to avoid problems like this:

int main() {
    int a = -1;
    unsigned int b = 1;
    if(a > b) {
        printf("According to my calculations %d IS greater than %d\n", a, b);
    }
}
$ cc main.c
$ ./a.out
According to my calculations -1 IS greater than 1

It's unfortunate that it's only caught by the compiler by using -Wextra or explicitly using -Wsign-compare (which is enabled by -Wextra).

Annotating the unused parameters with __unused at least makes us aware about the big number of function parameters we never use and, because it's annoying having this annotations, we might decide to get rid of them in the future.

Some changes were made to improve consistency, for example, the g++ compiler found a couple of instances of the code below (which is accepted by a C compiler):

enum Values {
    V1, V2, V3,
};
int main() {
    enum Values v = V1 | V2 | V3;
}

As the result of the expression is not a variant of the Values enum, it seems to me it doesn't make sense to store it in a variable of type enum Values.

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.

The protocol family (or domain) is handled as an int by the socket API.
We were not being consistent when handling it in our code and leading to
implicit type conversions from uint to int.
This function returns a NetplanDefType and -1 is not part of the
enum variants.
wowlan is used as a variable of type int, see handle_wowlan() where we
OR multiple variants of the enum NetplanWifiWowlanFlag and store it in
netdef->wowlan, producing a value that might not be a variant of the
enum.
All these options are reassigned few lines below with
NETPLAN_TRISTATE_UNSET which is the correct value for this type.
We initialize them with unsigned int values such G_MAXUINT.
sizeof() produces a value of type size_t.
The pointer subtraction returns a signed value. In this case, it will
never be negative but it's still an implicit conversion when we compare
the result with out_size.

It's still implicitly converting the type to size_t but at least we can
see it clearly now.
The 'const' qualifier is ignored when used on function return types.
Found by -Wextra.
Some structs initialization were missing members. Found by -Wextra.
Maybe this warning could just be disabled given the size of the diff,
but it seems useful to know how many of our functions have parameters
we never use.

The attribute __unused is defined in 'util-internal.h'.
It will add -Wextra to the build flags. It will enable some important
checks such as -Wsign-compare for example.
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.

Thank you, those are some very good catches.
It is tedious work to fix all those paper cuts, kudos for doing it and increasing the compiler warning level, to avoid such issues going forward!

lgtm.

@slyon slyon merged commit 806e764 into canonical:main Jul 19, 2023
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants