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

fix/simplify scripts/validate-c, fix *.c formatting #2860

Merged
merged 4 commits into from
Mar 28, 2021

Conversation

kolyshkin
Copy link
Contributor

@kolyshkin kolyshkin commented Mar 16, 2021

Apparently, scripts/validate-c is not working in CI (or maybe
maintainers ignored the failures from it?) -- current C code
gets some changes if we run recommended indent command on it.

This commit fixes this, simplifying things along the way.

In particular:

  1. Remove validate make target, add cfmt target that just runs
    indent on all *.c files in the repository (NOTE that *.h files
    are not included, as before).

    This may help a contributor to fix their code -- they just need
    to run "make cfmt" now instead of running "make validate" and
    copy-pasting the indent command and options from the hint.

  2. Split GHA validate/misc job into validate/release and validate/cfmt.
    The latter checks that the sources are not changed after make cfmt.

  3. Adds a few more options to indent. This was mostly motivated by
    trying to save the existing formatting as much as possible, minimizing
    the amount of changes indent produces.

    The added options are:

    • -il0: sets the offset for goto labels to 0 (currently all labels
      but one are not indented -- let's keep it that way);

    • -ppi2: sets the indentation for nested preprocessor directives
      to 2 spaces (same as in current code defining SYS_memfd_create);

    • -cp1: sets the indentation between #else / #endif and the
      following comment to 1 space.

  4. Reformat the code using the new indent options.

  5. Remove the now-unused script/{.validate,validate-c}.

v2: removed scripts/validate-c in favor of make cfmt and a GHA job; added a few indent options
v3: removed redundant comments from SYS_memfd_create defines

AkihiroSuda
AkihiroSuda previously approved these changes Mar 16, 2021
@kolyshkin
Copy link
Contributor Author

I'm going to play a bit more with indent and clang-format -- moving to draft for now.

@kolyshkin kolyshkin marked this pull request as draft March 17, 2021 01:25
This block apparently does nothing except for creating
a need for additional indentation. Remove it.

While at it, break a long line in this code.

No functional change.

Signed-off-by: Kir Kolyshkin <[email protected]>
@kolyshkin
Copy link
Contributor Author

Adding a test commit to check the newly added validate / cfmt job (extra vertical space, extra space between #endif and its comment, too long comment line):

@@ -132,7 +132,12 @@ int setns(int fd, int nstype)
 {
        return syscall(SYS_setns, fd, nstype);
 }
-#endif
+#endif      /* GLIBC < 2.14 */
+
+
+
+
+/* Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua. Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea commodo consequat. Duis aute irure dolor in reprehenderit in voluptate velit esse cillum dolore eu fugiat nulla pariatur. Excepteur sint occaecat cupidatat non proident, sunt in culpa qui officia deserunt mollit anim id est laborum. */
 
 static void write_log_with_info(const char *level, const char *function, int line, const char *format, ...)
 {

@kolyshkin
Copy link
Contributor Author

The validate / cfmt job fails as it should with the badly formatted input, so it works:

indent -linux -l120 -il0 -ppi2 -cp1 -T size_t -T jmp_buf libcontainer/nsenter/cloned_binary.c libcontainer/nsenter/nsexec.c tests/integration/testdata/dev_access_test.c tests/integration/testdata/seccomp_syscall_test1.c
diff --git a/libcontainer/nsenter/nsexec.c b/libcontainer/nsenter/nsexec.c
index 01ae8489..7d52e287 100644
--- a/libcontainer/nsenter/nsexec.c
+++ b/libcontainer/nsenter/nsexec.c
@@ -132,10 +132,7 @@ int setns(int fd, int nstype)
 {
 	return syscall(SYS_setns, fd, nstype);
 }
-#endif      /* GLIBC < 2.14 */
-
-
-
+#endif /* GLIBC < 2.14 */
 
 /* Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua. Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea commodo consequat. Duis aute irure dolor in reprehenderit in voluptate velit esse cillum dolore eu fugiat nulla pariatur. Excepteur sint occaecat cupidatat non proident, sunt in culpa qui officia deserunt mollit anim id est laborum. */


Error: Process completed with exit code 1.

Test commit removed.

@kolyshkin kolyshkin marked this pull request as ready for review March 17, 2021 21:14
@kolyshkin
Copy link
Contributor Author

No longer a draft. Note that validate / misc job is gone, replaced by validate / release and validate / cfmt, so it's not showing.

Once this PR is merged, I'll update the GH list of required checks.

@@ -372,35 +372,35 @@ static int seal_execfd(int *fd, int fdtype)
switch (fdtype) {
case EFD_MEMFD:
return fcntl(*fd, F_ADD_SEALS, RUNC_MEMFD_SEALS);
case EFD_FILE: {
Copy link
Member

Choose a reason for hiding this comment

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

Looking at the diff, this one surprised me (that it's removing the whitespace here,. but adding whitespace everywhere else)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is removing the whitespace here as I have removed the { } block (see description in the first commit).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry I was talking about a different hunk.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess indent has a flag for this one, too, but I'm too lazy to look for one, since this change only happens once, so adding that flag won't result in drastic changes in the patch size, and I don't want to have too many flags.

Comment on lines 80 to 81
# elif defined(__s390__) || defined(__s390x__)
// s390(x)
Copy link
Member

Choose a reason for hiding this comment

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

This one doesn't look right? I'd expected the old one here (also notice that it's using tabs to indent the comment, whereas lines above use spaces for indentation (but tabs for the last comments? (I'll leave a separate comment about that)

Screenshot 2021-03-18 at 10 00 24

So there's six tabs in front of the comment; is it trying to fit it into columns?

Anticipating something like;

  • line 78, being slightly shorter; comment starts at position 49 (so don't add tabs to align)
  • line 80: comment started at position 50/51;
    • -> "just beyond the cut"
    • -> add 6 tabs (24 positions?) to put it in the next column
    • -> now line-width is at 50+24+10 (comment-length)
    • -> which is > 80 characters, so wrap the line

That said, the comment at line 376 did not get reformatted/wrapped, and is at (assuming tab=4) 100 positions wide, so not sure.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't like it either, but this is the way indent works (trying to bring the adjacent comments to the same column, and, I suspect, have some predefined preferred column position for such one-line comments).

Overall it makes sense to me -- this is how it looks like for me:

Screenshot from 2021-03-18 16-43-41

(note that ppc and and s390 are aligned -- I suspect they are not for you as you have tabstop != 8)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

BTW one way to fix this is remove those comments (they are kinda redundant anyway)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed // $ARCH comments

# elif defined(__ppc__) || defined(__ppc64__) // ppc + ppc64
# elif defined(__ppc__) || defined(__ppc64__)// ppc + ppc64
Copy link
Member

Choose a reason for hiding this comment

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

This one doesn't look right; why is it removing the whitespace?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See above

# define SYS_memfd_create 385
# elif defined(__aarch64__) // arm64
# elif defined(__aarch64__) // arm64
Copy link
Member

Choose a reason for hiding this comment

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

I should probbably point this out as well, as it's difficult to see on GitHub;

#    elif defined(__aarch64__)	// arm64
  ^                           ^
  spaces                      tab

Perhaps that's standard, but it felt a bit odd that the code uses tabs (both for indentation and for aligning?), but in these blocks, spaces are used for indentation, which felt a bit odd (perhaps it's convention/standard?). Of course, I'm also used to Go's formatting now (tabs for indentation, but spaces for aligning).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Spaces are used to indent the nested preprocessor directives as described above; let me quote myself

Adds a few more options to indent. This was mostly motivated by
trying to save the existing formatting as much as possible, minimizing
the amount of changes indent produces.
...

  • -ppi2: sets the indentation for nested preprocessor directives
    to 2 spaces (same as in current code defining SYS_memfd_create);

Current code use three four different ways to indent the nested cpp directives
(no indentation, tabs, two spaces, spaces and tabs). I have settled on two spaces.

As I said the idea was to minimize the amount of changes, and yet keep the formatting
consistent. I will be happy to reconsider (there's also an option to ignore cpp indentation).

libcontainer/nsenter/cloned_binary.c Show resolved Hide resolved
Remove comments with architectures when defining SYS_memfd_create,
as they are redundant, and indent has a funny way of indenting them.

Signed-off-by: Kir Kolyshkin <[email protected]>
Apparently, scripts/validate-c is not working in CI (or maybe
maintainers ignored the failures from it) -- current C code
gets some changes if we run indent on it.

This commit fixes this, simplifying things along the way.

 In particular:

1. Remove "validate" make target, add "cfmt" target that just runs
   indent on all *.c files in the repository (NOTE that *.h files
   are not included, as before).

   This may help a contributor to fix their code -- they just need
   to run "make cfmt" now instead of running "make validate" and
   copy-pasting the indent command and options from the hint.

2. Split GHA validate/misc into validate/release and validate/cfmt.
   The latter checks that the sources are not changed after "make cfmt".

3. Adds a few more options to indent. This was mostly motivated by
   trying to save the existing formatting, minimizing the amount of
   changes indent produces.

   The new options are:

   * -il0: sets the offset for goto labels to 0 (currently all labels
     but one are not indented -- let's keep it that way);

   * -ppi2: sets the indentation for nested preprocessor directives
     to 2 spaces (same as it is done in "SYS_memfd_create" defines);

   * -cp1: sets the indentation between #else / #endif and the
     following comment to 1 space.

4. Reformat the code using the new indent options.

5. Remove the now-unused script/{.validate,validate-c}.

Signed-off-by: Kir Kolyshkin <[email protected]>
They were not aligned, and the last two had spaces not tabs.

Signed-off-by: Kir Kolyshkin <[email protected]>
#define SETGROUP_ATTR 27285
#define OOM_SCORE_ADJ_ATTR 27286
#define ROOTLESS_EUID_ATTR 27287
#define UIDMAPPATH_ATTR 27288
#define GIDMAPPATH_ATTR 27289
#define UIDMAPPATH_ATTR 27288
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the alignment still off for the last two?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was a mix of tabs and spaces, now it's just tabs and it looks aligned for tabstep=8.

GH does not use tabstep=8 so probably why it looks skewed here.

Copy link
Member

Choose a reason for hiding this comment

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

Looks like you can adjust with ?ts=<width> when viewing; https://github.com/tiimgreen/github-cheat-sheet#adjust-tab-space

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@thaJeztah does not seem to work when viewing diffs :-\

Copy link
Member

Choose a reason for hiding this comment

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

😕😕😞😞😩

Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM

I did see one line still having some alignment issues in nsexec.c; probably not a showstopper, but leaving screenshots from GitHub, my IDE, and a command-line editor;

Screenshot 2021-03-27 at 19 19 27

Screenshot 2021-03-27 at 19 19 41

Screenshot 2021-03-27 at 19 21 07

@kolyshkin kolyshkin merged commit 2400e5e into opencontainers:master Mar 28, 2021
@kolyshkin
Copy link
Contributor Author

@thaJeztah fixing this one in #2836

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants