[PATCH v3 3/7] virdnsmasq: Lookup DNSMASQ in PATH

[Michal Prívozník]

On 1/14/22 16:29, Andrea Bolognani wrote:
> On Wed, Jan 12, 2022 at 09:47:54AM +0100, Michal Privoznik wrote:
>> With this code in place, the virFileIsExecutable() check can be
>> removed from dnsmasqCapsRefreshInternal() because
>> virFindFileInPath() already made sure the binary is executable.
>>
>> But introducing virFileIsExecutable() means we have to mock it in
> 
> I believe you meant virFindFileInPath() here.

Oh, of course.

> 
>> +++ b/src/util/virdnsmasq.c
>> @@ -702,14 +692,20 @@ dnsmasqCapsRefreshInternal(dnsmasqCaps *caps, bool force)
>>  static dnsmasqCaps *
>>  dnsmasqCapsNewEmpty(void)
>>  {
>> -    dnsmasqCaps *caps;
>> +    g_autoptr(dnsmasqCaps) caps = NULL;
>>
>>      if (dnsmasqCapsInitialize() < 0)
>>          return NULL;
>>      if (!(caps = virObjectNew(dnsmasqCapsClass)))
>>          return NULL;
>> -    caps->binaryPath = g_strdup(DNSMASQ);
>> -    return caps;
>> +
>> +    if (!(caps->binaryPath = virFindFileInPath(DNSMASQ))) {
>> +        virReportSystemError(ENOENT, "%s",
>> +                             _("Unable to find 'dnsmasq' binary in $PATH"));
>> +        return NULL;
>> +    }
>> +
>> +    return g_steal_pointer(&caps);
>>  }
> 
> Can you please squash the g_autoptr conversion into the previous
> patch?
> 

Fair enough. I thought this is okay, since it's only this commit that
introduces an alternative return path from dnsmasqCapsNewEmpty(). But I
don't mind either way. Consider it done.

>> +++ b/tests/networkmock.c
>> @@ -0,0 +1,30 @@
>> +/*
>> + * This library is free software; you can redistribute it and/or
>> + * modify it under the terms of the GNU Lesser General Public
>> + * License as published by the Free Software Foundation; either
>> + * version 2.1 of the License, or (at your option) any later version.
>> + *
>> + * This library is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
>> + * Lesser General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU Lesser General Public
>> + * License along with this library.  If not, see
>> + * <http://www.gnu.org/licenses/>.
>> + */
> 
> Missing copyright information. Also can we have an SPDX tag instead
> of the full license blurb? See tests/virhostdevmock.c for an example
> of what I have in mind.

Okay.

> 
>> +char *
>> +virFindFileInPath(const char *file)
>> +{
>> +    if (file && g_strrstr(file, "dnsmasq"))
>> +        return g_strdup(file);
> 
> This mock is somewhat inaccurate because, if dnsmasq was not found at
> configure time, then DNSMASQ will be defined to "dnsmasq" and the
> mocked function will return that string instead of an absolute path.
> Clearly this doesn't break the test case, but it still feels off.
> 
> I wonder if we should drop the configure time detection for dnsmasq
> altogether, same as we've done for other optional binaries, always
> define DNSMASQ to be "dnsmasq", and here do
> 
>   if (STREQ_NULLABLE(file, "dnsmasq"))
>       return g_strdup("/usr/bin/dnsmasq");
> 
> What do you think?
> 

Yup, will do. Except s/bin/sbin/ because that's where dnsmasq usually is.

Michal