Re: [libvirt PATCH 10/11] vircommand: Remove NULL check in virCommandAddArg

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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.




[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]

  Powered by Linux