On Thu, Jan 28, 2021 at 15:21:28 +0100, Tim Wiederhake wrote: > On Thu, 2021-01-28 at 11:54 +0100, Peter Krempa wrote: > > On Thu, Jan 28, 2021 at 11:24:40 +0100, Tim Wiederhake wrote: > > > `val` is declared `ATTRIBUTE_NONNULL`. > > > > Please see: > > > > https://gitlab.com/libvirt/libvirt/-/blob/master/src/internal.h#L127 > > > > ATTRIBUTE_NONNULL is unfortunately lead to wrong optimizations done > > by > > gcc. > > > > > Found by clang-tidy's "clang-diagnostic-tautological-pointer- > > > compare" > > > check. > > > > > > Signed-off-by: Tim Wiederhake <twiederh@xxxxxxxxxx> > > > --- > > > src/util/vircommand.c | 5 ----- > > > 1 file changed, 5 deletions(-) > > > > > > diff --git a/src/util/vircommand.c b/src/util/vircommand.c > > > index c3a98bbeac..223010c6aa 100644 > > > --- a/src/util/vircommand.c > > > +++ b/src/util/vircommand.c > > > @@ -1525,11 +1525,6 @@ virCommandAddArg(virCommandPtr cmd, const > > > char *val) > > > if (!cmd || cmd->has_error) > > > return; > > > > > > - if (val == NULL) { > > > - cmd->has_error = EINVAL; > > > - return; > > > - } > > > > So this might have actual value. > > > > Hm, I see. I will remove this patch from the series for now. A bit > frustrating though, if you compare with, as a random example, > "virCommandGetGID", which is declared in [1] as "ATTRIBUTE_NONNULL" for > argument "cmd", which is subsequently dereferenced without additional > NULL check in [2]. I consider the difference here to be on libvirt-semantic level rather than language-semantic level. Libvirt's semantics dictate that virCommandAddArg must not be used with NULL 'val' since adding a NULL argument wouldn't make sense. We similarly check this in virCommandAddArgSet that the set of arguments to add is non-empty. (You can also see a ATTRIBUTE_NONNULL there, but it works differently). The language semantics of ATTRIBUTE_NONNULL dictate that the function must not be called with NULL argument, which in case of virCommandAddArg is equivalent with the desired libvirt semantics. Unfortunately due to the weird handling in the compiler (not static analyzer) the compiler doesn't guarantee anything about the argument if ATTRIBUTE_NONNULL is used. Since the explicit check of val is required to ensure the semantics despite the uselessness of ATTRIBUTE_NONNULL when used in real compile case, we must keep the check in. So the question is now whether to keep it marked as ATTRIBUTE_NONNULL, or just do it on documentation level and appease the static analyzer by dropping ATTRIBUTE_NONNULL.
