Since the empty_to_null() function was "macrofied", we need to use a bit
of black magic to make Coccinelle avoid running the transformation on
the macro itself.
Follow-up to ef2409cbde.
-1 was used everywhere, but -EBADF or -EBADFD started being used in various
places. Let's make things consistent in the new style.
Note that there are two candidates:
EBADF 9 Bad file descriptor
EBADFD 77 File descriptor in bad state
Since we're initializating the fd, we're just assigning a value that means
"no fd yet", so it's just a bad file descriptor, and the first errno fits
better. If instead we had a valid file descriptor that became invalid because
of some operation or state change, the other errno would fit better.
In some places, initialization is dropped if unnecessary.
RUN_WITH_UMASK was initially conceived for spawning externals progs with the
umask set. But nowadays we use it various syscalls and stuff that doesn't "run"
anything, so the "RUN_" prefix has outlived its usefulness.
This was a trivial wrapper that didn't provide any added value. With more
complicated structures like strvs, hashmaps, sets, and arrays, it is possible
to have an empty container. But in case of a list, the list is empty only when
the head is missing.
Also, we generally want the positive condition, so we replace many
if (!LIST_IS_EMPTY(x)) with just if (x).
Let's define two helpers strdupa_safe() + strndupa_safe() which do the
same as their non-safe counterparts, except that they abort if called
with allocations larger than ALLOCA_MAX.
This should ensure that all our alloca() based allocations are subject
to this limit.
afaics glibc offers three alloca() based APIs: alloca() itself,
strndupa() + strdupa(). With this we have now replacements for all of
them, that take the limit into account.
* flag-set.cocci: perform the transformation only if the second
argument is a constant
* sd-journal/lookup3.c: skip the cocci completely for this file, since
it's not "ours"
* strjoina.cocci: skip the transformation on the "test_strjoina" test,
since it intentionally tests the "incorrect" expression we're trying to
transform (the same thing was already done in strjoin.cocci)
to drop braces around single-line if statements. Also, prefix it with
zz- so it runs as the last one, so it's able to fix stuff tweaked by
previous transformations.
Coccinelle can't do this automagically and requires we supply it
respective header files. Unfortunately, the option for this
(--macro-file=) can be used only once, so let's create our own
macro file by collecting macros needed for the semantic parser
to be happy.
The original issue with this transformation was that we were replacing
the whole if statement instead of just the expression inside. That
caused the code to be weirdly formatted, as Coccinelle put a new block
around each replaced if statement.
This version replaces just the inner expression if it's in its incorrect
form, otherwise it just accepts it (to avoid recursion).
My former dumb me didn't read the documentation properly, so with the
introduction of custom isomorphisms I caused two issues:
1) Masked all standard isomorphisms defined by Coccinelle
2) Replace the original issue with a completely new one
Patch contains a coccinelle script, but it only works in some cases. Many
parts were converted by hand.
Note: I did not fix errors in return value handing. This will be done separate
to keep the patch comprehensible. No functional change is intended in this
patch.
Some transformations generate results we don't want to keep, so
let's disable such transformations for specific files.
Also, disable const-strlen.cocci everywhere, as the STRLEN macro has a
pretty limited scope, so the transformation generates false positives in
most cases.
There's no point in running these transformation for certain files,
mainly anything from src/boot/efi and src/shared/linux, as this code
doesn't have access to our internal utility functions
For example, following transformation:
- isempty(s) ? NULL : s
+ empty_to_null(s)
would get applied to the empty_to_null function itself as well,
causing an infinite recursion, like:
--- src/basic/string-util.h
+++ /tmp/cocci-output-307-9f76e6-string-util.h
@@ -50,11 +50,11 @@ static inline bool isempty(const char *p
}
static inline const char *empty_to_null(const char *p) {
- return isempty(p) ? NULL : p;
+ return empty_to_null(p);
}
Let's avoid that by checking the current match position
For example, the following transformation:
- sizeof(s)-1
+ STRLEN(s)
would replace sizeof by STRLEN even in the STRLEN macro definition
itself, which generates following nonsensical patch:
--- src/basic/macro.h
+++ /tmp/cocci-output-8753-b50773-macro.h
@@ -182,7 +182,7 @@ static inline unsigned long ALIGN_POWER2
* Contrary to strlen(), this is a constant expression.
* @x: a string literal.
*/
-#define STRLEN(x) (sizeof(""x"") - 1)
+#define STRLEN(x) (STRLEN("" x ""))
/*
* container_of - cast a member of a structure out to the containing structure
Let's exclude the macro itself from the transformation to avoid this
The `coccinelle/take-fd.cocci` transformation file attempts to rewrite
r = fd;
fd = -1;
to
r = TAKE_FD(fd);
Unfortunately, using `identifier` or `idexpression` as a metavariable
type in this case wouldn't match more complex location descriptions,
like:
x->fd = fd
fd = -1;
Using 'expression' metavariable type generates false positives,
as you can't specify scope of such expression. The only real example
from the current codebase is the global 'errno' variable, which results
in following patch generated by `spatch`:
--- src/basic/errno-util.h
+++ /tmp/cocci-output-28263-971baa-errno-util.h
@@ -15,8 +15,7 @@ static inline void _reset_errno_(int *sa
#define UNPROTECT_ERRNO \
do { \
- errno = _saved_errno_; \
- _saved_errno_ = -1; \
+ errno = TAKE_FD(_saved_errno_); \
} while (false)
static inline int negative_errno(void) {
Let's explicitly state that the matched expression should not equal
'errno' to avoid this. It's not particularly nice, but it should be
enough, at least for now.
Coccinelle needs a custom isomorphism file with rules (isomorphisms) how
to correctly rewrite conditions with explicit NULL checks (i.e.
if (ptr == NULL)) to their shorter form (i.e. if (!ptr)). Coccinelle
already contains such isomorphisms in its default .iso file, however,
they're in the opposite direction, which results in useless output from
coccinelle/equals-null.cocci.
With this fix, `spatch` should no longer report patches like:
@@ -628,8 +628,9 @@ static int path_deserialize_item(Unit *u
f = path_result_from_string(value);
if (f < 0)
log_unit_debug(u, "Failed to parse result value: %s", value);
- else if (f != PATH_SUCCESS)
- p->result = f;
+ else {if (f != PATH_SUCCESS)
+ p->result = f;
+ }
} else
log_unit_debug(u, "Unknown serialization key: %s", key);
This is partially a refactoring, but also makes many more places use
unlocked operations implicitly, i.e. all users of fopen_temporary().
AFAICT, the uses are always for short-lived files which are not shared
externally, and are just used within the same context. Locking is not
necessary.
We had all kinds of indentation: 2 sp, 3 sp, 4 sp, 8 sp, and mixed.
4 sp was the most common, in particular the majority of scripts under test/
used that. Let's standarize on 4 sp, because many commandlines are long and
there's a lot of nesting, and with 8sp indentation less stuff fits. 4 sp
also seems to be the default indentation, so this will make it less likely
that people will mess up if they don't load the editor config. (I think people
often use vi, and vi has no support to load project-wide configuration
automatically. We distribute a .vimrc file, but it is not loaded by default,
and even the instructions in it seem to discourage its use for security
reasons.)
Also remove the few vim config lines that were left. We should either have them
on all files, or none.
Also remove some strange stuff like '#!/bin/env bash', yikes.
