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
