9:44 a.m.
This is an alternative implementation to Olaf's original patch posted
here:
https://listman.redhat.com/archives/libvir-list/2021-August/msg00273.html
I find my patches to be a bit cleaner, but either way we should merge
his or mine.
Michal Prívozník (3):
virdnsmasq: Drop @binaryPath argument from dnsmasqCapsNewEmpty()
virdnsmasq: Lookup DNSMASQ in PATH
virdnsmasq: Require non NULL @caps in dnsmasqCapsGetBinaryPath()
src/util/virdnsmasq.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
--
2.34.1
9:44 a.m.
New subject: [PATCH 1/3] virdnsmasq: Drop @binaryPath argument from dnsmasqCapsNewEmpty()
Both callers of dnsmasqCapsNewEmpty() pass DNSMASQ as an argument
which is then fed to a ternary operator which looks like this
(after substitution).
DNSMASQ ? DNSMASQ : DNSMASQ
While I like tautologies, the code can be simplified by dropping
the argument.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/util/virdnsmasq.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/src/util/virdnsmasq.c b/src/util/virdnsmasq.c
index e5edec2b64..d304929d51 100644
--- a/src/util/virdnsmasq.c
+++ b/src/util/virdnsmasq.c
@@ -700,7 +700,7 @@ dnsmasqCapsRefreshInternal(dnsmasqCaps *caps, bool force)
}
static dnsmasqCaps *
-dnsmasqCapsNewEmpty(const char *binaryPath)
+dnsmasqCapsNewEmpty(void)
{
dnsmasqCaps *caps;
@@ -708,14 +708,14 @@ dnsmasqCapsNewEmpty(const char *binaryPath)
return NULL;
if (!(caps = virObjectNew(dnsmasqCapsClass)))
return NULL;
- caps->binaryPath = g_strdup(binaryPath ? binaryPath : DNSMASQ);
+ caps->binaryPath = g_strdup(DNSMASQ);
return caps;
}
dnsmasqCaps *
dnsmasqCapsNewFromBuffer(const char *buf)
{
- dnsmasqCaps *caps = dnsmasqCapsNewEmpty(DNSMASQ);
+ dnsmasqCaps *caps = dnsmasqCapsNewEmpty();
if (!caps)
return NULL;
@@ -730,7 +730,7 @@ dnsmasqCapsNewFromBuffer(const char *buf)
dnsmasqCaps *
dnsmasqCapsNewFromBinary(void)
{
- dnsmasqCaps *caps = dnsmasqCapsNewEmpty(DNSMASQ);
+ dnsmasqCaps *caps = dnsmasqCapsNewEmpty();
if (!caps)
return NULL;
--
2.34.1
11:39 a.m.
New subject: [PATCH 1/3] virdnsmasq: Drop @binaryPath argument from dnsmasqCapsNewEmpty()
On Mon, Jan 10, 2022 at 04:44:53PM +0100, Michal Privoznik wrote:
Both callers of dnsmasqCapsNewEmpty() pass DNSMASQ as an argument
which is then fed to a ternary operator which looks like this
(after substitution).
DNSMASQ ? DNSMASQ : DNSMASQ
While I like tautologies, the code can be simplified by dropping
the argument.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/util/virdnsmasq.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
Reviewed-by: Andrea Bolognani <abologna(a)redhat.com>
--
Andrea Bolognani / Red Hat / Virtualization
9:44 a.m.
New subject: [PATCH 2/3] virdnsmasq: Lookup DNSMASQ in PATH
While it's true that our virCommand subsystem is happy with
non-absolute paths, the dnsmasq capability code is not. For
instance, it does call stat() over the binary to learn its mtime
(and thus decide whether capabilities need to be fetched again or
not).
Therefore, when constructing the capabilities structure look up
the binary path. If DNSMASQ already contains an absolute path
then it is returned (and virFindFileInPath() is a NOP).
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/util/virdnsmasq.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/util/virdnsmasq.c b/src/util/virdnsmasq.c
index d304929d51..b6ccb9d0a4 100644
--- a/src/util/virdnsmasq.c
+++ b/src/util/virdnsmasq.c
@@ -708,7 +708,7 @@ dnsmasqCapsNewEmpty(void)
return NULL;
if (!(caps = virObjectNew(dnsmasqCapsClass)))
return NULL;
- caps->binaryPath = g_strdup(DNSMASQ);
+ caps->binaryPath = virFindFileInPath(DNSMASQ);
return caps;
}
--
2.34.1
11:41 a.m.
New subject: [PATCH 2/3] virdnsmasq: Lookup DNSMASQ in PATH
On Mon, Jan 10, 2022 at 04:44:55PM +0100, Michal Privoznik wrote:
While it's true that our virCommand subsystem is happy with
non-absolute paths, the dnsmasq capability code is not. For
instance, it does call stat() over the binary to learn its mtime
(and thus decide whether capabilities need to be fetched again or
not).
Therefore, when constructing the capabilities structure look up
the binary path. If DNSMASQ already contains an absolute path
then it is returned (and virFindFileInPath() is a NOP).
Saying that virFindFileInPath() is a NOP is not quite correct: if you
pass an absolute path, it will return a copy. So I'd rewrite the
above as
If DNSMASQ already contains an absolute path then
virFindFileInPath() will simply return a copy.
Reviewed-by: Andrea Bolognani <abologna(a)redhat.com>
--
Andrea Bolognani / Red Hat / Virtualization
2:28 a.m.
New subject: [PATCH 2/3] virdnsmasq: Lookup DNSMASQ in PATH
On 1/10/22 18:41, Andrea Bolognani wrote:
On Mon, Jan 10, 2022 at 04:44:55PM +0100, Michal Privoznik wrote:
> While it's true that our virCommand subsystem is happy with
> non-absolute paths, the dnsmasq capability code is not. For
> instance, it does call stat() over the binary to learn its mtime
> (and thus decide whether capabilities need to be fetched again or
> not).
>
> Therefore, when constructing the capabilities structure look up
> the binary path. If DNSMASQ already contains an absolute path
> then it is returned (and virFindFileInPath() is a NOP).
Saying that virFindFileInPath() is a NOP is not quite correct: if you
pass an absolute path, it will return a copy. So I'd rewrite the
above as
If DNSMASQ already contains an absolute path then
virFindFileInPath() will simply return a copy.
Yes, this makes sense.
Reviewed-by: Andrea Bolognani <abologna(a)redhat.com>
Thanks, but as I was looking through virFindFileInPath() I've noticed
that if the file exists but is not executable then NULL is returned, so
we will need a fallback case. Effectively this needs to be squashed in:
diff --git c/src/util/virdnsmasq.c w/src/util/virdnsmasq.c
index b6ccb9d0a4..7792a65bc3 100644
--- c/src/util/virdnsmasq.c
+++ w/src/util/virdnsmasq.c
@@ -703,12 +703,17 @@ static dnsmasqCaps *
dnsmasqCapsNewEmpty(void)
{
dnsmasqCaps *caps;
+ g_autofree char *binaryPath = NULL;
if (dnsmasqCapsInitialize() < 0)
return NULL;
if (!(caps = virObjectNew(dnsmasqCapsClass)))
return NULL;
- caps->binaryPath = virFindFileInPath(DNSMASQ);
+
+ if (!(binaryPath = virFindFileInPath(DNSMASQ)))
+ binaryPath = g_strdup(DNSMASQ);
+
+ caps->binaryPath = g_steal_pointer(&binaryPath);
return caps;
}
I'll post v2 of this patch shortly.
Michal
3:45 a.m.
New subject: [PATCH 2/3] virdnsmasq: Lookup DNSMASQ in PATH
On Tue, Jan 11, 2022 at 09:28:45AM +0100, Michal Prívozník wrote:
Thanks, but as I was looking through virFindFileInPath() I've
noticed
that if the file exists but is not executable then NULL is returned, so
we will need a fallback case.
Right. But considering that dnsmasqCapsRefreshInternal() includes a
check for whether or not the file is executable, and an error is
returned if it's not, wouldn't it make more sense to remove that
check and simply exit early if virFindFileInPath() returns NULL?
diff --git a/src/util/virdnsmasq.c b/src/util/virdnsmasq.c
index a869b398c7..e31e78224d 100644
--- a/src/util/virdnsmasq.c
+++ b/src/util/virdnsmasq.c
@@ -670,16 +670,6 @@ dnsmasqCapsRefreshInternal(dnsmasqCaps *caps, bool force)
return 0;
caps->mtime = sb.st_mtime;
- /* Make sure the binary we are about to try exec'ing exists.
- * Technically we could catch the exec() failure, but that's
- * in a sub-process so it's hard to feed back a useful error.
- */
- if (!virFileIsExecutable(caps->binaryPath)) {
- virReportSystemError(errno, _("dnsmasq binary %s is not executable"),
- caps->binaryPath);
- return -1;
- }
-
vercmd = virCommandNewArgList(caps->binaryPath, "--version", NULL);
virCommandSetOutputBuffer(vercmd, &version);
virCommandAddEnvPassCommon(vercmd);
@@ -708,7 +698,6 @@ dnsmasqCapsNewEmpty(void)
return NULL;
if (!(caps = virObjectNew(dnsmasqCapsClass)))
return NULL;
- caps->binaryPath = virFindFileInPath(DNSMASQ);
return caps;
}
@@ -720,6 +709,8 @@ dnsmasqCapsNewFromBuffer(const char *buf)
if (!caps)
return NULL;
+ caps->binaryPath = g_strdup(DNSMASQ);
+
if (dnsmasqCapsSetFromBuffer(caps, buf) < 0) {
virObjectUnref(caps);
return NULL;
@@ -735,6 +726,11 @@ dnsmasqCapsNewFromBinary(void)
if (!caps)
return NULL;
+ if (!(caps->binaryPath = virFindFileInPath(DNSMASQ))) {
+ virObjectUnref(caps);
+ return NULL;
+ }
+
if (dnsmasqCapsRefreshInternal(caps, true) < 0) {
virObjectUnref(caps);
return NULL;
--
Andrea Bolognani / Red Hat / Virtualization
3:59 a.m.
New subject: [PATCH 2/3] virdnsmasq: Lookup DNSMASQ in PATH
On Tue, Jan 11, 2022 at 09:28:45AM +0100, Michal Prívozník wrote:
On 1/10/22 18:41, Andrea Bolognani wrote:
> On Mon, Jan 10, 2022 at 04:44:55PM +0100, Michal Privoznik wrote:
>> While it's true that our virCommand subsystem is happy with
>> non-absolute paths, the dnsmasq capability code is not. For
>> instance, it does call stat() over the binary to learn its mtime
>> (and thus decide whether capabilities need to be fetched again or
>> not).
>>
>> Therefore, when constructing the capabilities structure look up
>> the binary path. If DNSMASQ already contains an absolute path
>> then it is returned (and virFindFileInPath() is a NOP).
>
> Saying that virFindFileInPath() is a NOP is not quite correct: if you
> pass an absolute path, it will return a copy. So I'd rewrite the
> above as
>
> If DNSMASQ already contains an absolute path then
> virFindFileInPath() will simply return a copy.
>
Yes, this makes sense.
> Reviewed-by: Andrea Bolognani <abologna(a)redhat.com>
>
Thanks, but as I was looking through virFindFileInPath() I've noticed
that if the file exists but is not executable then NULL is returned, so
we will need a fallback case. Effectively this needs to be squashed in:
Why do we need a fallback ? If someone has put 'dnsmasq' in $PATH
without the execute bit set, surely that's just a broken deployment
and returning NULL is correct.
diff --git c/src/util/virdnsmasq.c w/src/util/virdnsmasq.c
index b6ccb9d0a4..7792a65bc3 100644
--- c/src/util/virdnsmasq.c
+++ w/src/util/virdnsmasq.c
@@ -703,12 +703,17 @@ static dnsmasqCaps *
dnsmasqCapsNewEmpty(void)
{
dnsmasqCaps *caps;
+ g_autofree char *binaryPath = NULL;
if (dnsmasqCapsInitialize() < 0)
return NULL;
if (!(caps = virObjectNew(dnsmasqCapsClass)))
return NULL;
- caps->binaryPath = virFindFileInPath(DNSMASQ);
+
+ if (!(binaryPath = virFindFileInPath(DNSMASQ)))
+ binaryPath = g_strdup(DNSMASQ);
+
+ caps->binaryPath = g_steal_pointer(&binaryPath);
return caps;
}
I'll post v2 of this patch shortly.
Michal
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
4:18 a.m.
New subject: [PATCH 2/3] virdnsmasq: Lookup DNSMASQ in PATH
On Tue, Jan 11, 2022 at 09:59:06AM +0000, Daniel P. Berrangé wrote:
On Tue, Jan 11, 2022 at 09:28:45AM +0100, Michal Prívozník wrote:
> Thanks, but as I was looking through virFindFileInPath() I've noticed
> that if the file exists but is not executable then NULL is returned, so
> we will need a fallback case. Effectively this needs to be squashed in:
Why do we need a fallback ? If someone has put 'dnsmasq' in $PATH
without the execute bit set, surely that's just a broken deployment
and returning NULL is correct.
Agreed in general, but we want the test suite to still pass even if
dnsmasq is not installed on the build machine. The approach I
suggested in my other message would achieve that.
--
Andrea Bolognani / Red Hat / Virtualization
4:24 a.m.
New subject: [PATCH 2/3] virdnsmasq: Lookup DNSMASQ in PATH
On Tue, Jan 11, 2022 at 10:18:23AM +0000, Andrea Bolognani wrote:
On Tue, Jan 11, 2022 at 09:59:06AM +0000, Daniel P. Berrangé wrote:
> On Tue, Jan 11, 2022 at 09:28:45AM +0100, Michal Prívozník wrote:
> > Thanks, but as I was looking through virFindFileInPath() I've noticed
> > that if the file exists but is not executable then NULL is returned, so
> > we will need a fallback case. Effectively this needs to be squashed in:
>
> Why do we need a fallback ? If someone has put 'dnsmasq' in $PATH
> without the execute bit set, surely that's just a broken deployment
> and returning NULL is correct.
Agreed in general, but we want the test suite to still pass even if
dnsmasq is not installed on the build machine. The approach I
suggested in my other message would achieve that.
Shouldn't we just mock the virFindFileInPath call in the test suite
to return whatever fixed binary path we need, so we don't have to
modify the driver code with special logic for tests.
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
9:45 a.m.
New subject: [PATCH 2/3] virdnsmasq: Lookup DNSMASQ in PATH
On Tue, Jan 11, 2022 at 10:24:00AM +0000, Daniel P. Berrangé wrote:
On Tue, Jan 11, 2022 at 10:18:23AM +0000, Andrea Bolognani wrote:
> On Tue, Jan 11, 2022 at 09:59:06AM +0000, Daniel P. Berrangé wrote:
> > Why do we need a fallback ? If someone has put 'dnsmasq' in $PATH
> > without the execute bit set, surely that's just a broken deployment
> > and returning NULL is correct.
>
> Agreed in general, but we want the test suite to still pass even if
> dnsmasq is not installed on the build machine. The approach I
> suggested in my other message would achieve that.
Shouldn't we just mock the virFindFileInPath call in the test suite
to return whatever fixed binary path we need, so we don't have to
modify the driver code with special logic for tests.
We'd have to also mock *running* the binary, as we get some
information out of that. Probably a better long-term approach, but I
don't think it should necessarily be a blocker for this series, which
already represents an improvement over the status quo.
--
Andrea Bolognani / Red Hat / Virtualization
9:44 a.m.
New subject: [PATCH 3/3] virdnsmasq: Require non NULL @caps in dnsmasqCapsGetBinaryPath()
First observation: There is no way that caps->binaryPath can be
NULL. Second observation: There is no caller that passes NULL.
Let's drop the ternary operator and access @caps directly.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/util/virdnsmasq.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/util/virdnsmasq.c b/src/util/virdnsmasq.c
index b6ccb9d0a4..a869b398c7 100644
--- a/src/util/virdnsmasq.c
+++ b/src/util/virdnsmasq.c
@@ -745,7 +745,7 @@ dnsmasqCapsNewFromBinary(void)
const char *
dnsmasqCapsGetBinaryPath(dnsmasqCaps *caps)
{
- return caps ? caps->binaryPath : DNSMASQ;
+ return caps->binaryPath;
}
/** dnsmasqDhcpHostsToString:
--
2.34.1
11:41 a.m.
New subject: [PATCH 3/3] virdnsmasq: Require non NULL @caps in dnsmasqCapsGetBinaryPath()
On Mon, Jan 10, 2022 at 04:44:57PM +0100, Michal Privoznik wrote:
First observation: There is no way that caps->binaryPath can be
NULL. Second observation: There is no caller that passes NULL.
Let's drop the ternary operator and access @caps directly.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/util/virdnsmasq.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
Reviewed-by: Andrea Bolognani <abologna(a)redhat.com>
--
Andrea Bolognani / Red Hat / Virtualization
2:43 a.m.
New subject: [PATCH v2 2/3] virdnsmasq: Lookup DNSMASQ in PATH
While it's true that our virCommand subsystem is happy with
non-absolute paths, the dnsmasq capability code is not. For
instance, it does call stat() over the binary to learn its mtime
(and thus decide whether capabilities need to be fetched again or
not).
Therefore, when constructing the capabilities structure look up
the binary path. If DNSMASQ already contains an absolute path
then virFindFileInPath() will simply return a copy. But, if we
failed to find the binary in $PATH then do not report error.
Either the dnsmasqCapsNewEmpty() function is called from
dnsmasqCapsNewFromBinary() which will later validate the path and
return appropriate error, or the function is called from a test
suite (via dnsmasqCapsNewFromBuffer()) and the caps are parsed
from a fixed string.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
Diff to v1:
- Per my experiments we need a fallback case when DNSMASQ exists but is
not executable.
At any rate, this whole code could use cleanup, but let's save that for
after the release.
src/util/virdnsmasq.c | 11 ++++++++++-
1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/src/util/virdnsmasq.c b/src/util/virdnsmasq.c
index d304929d51..8ddc2d0b78 100644
--- a/src/util/virdnsmasq.c
+++ b/src/util/virdnsmasq.c
@@ -703,12 +703,21 @@ static dnsmasqCaps *
dnsmasqCapsNewEmpty(void)
{
dnsmasqCaps *caps;
+ g_autofree char *binaryPath = NULL;
if (dnsmasqCapsInitialize() < 0)
return NULL;
if (!(caps = virObjectNew(dnsmasqCapsClass)))
return NULL;
- caps->binaryPath = g_strdup(DNSMASQ);
+
+ if (!(binaryPath = virFindFileInPath(DNSMASQ))) {
+ /* Don't report error here because we might be running
+ * from a test suite an initializing capabilities from
+ * a buffer (dnsmasqCapsNewFromBuffer()). */
+ binaryPath = g_strdup(DNSMASQ);
+ }
+
+ caps->binaryPath = g_steal_pointer(&binaryPath);
return caps;
}
--
2.34.1
1046
days inactive
1047
days old
13 comments
4 participants
participants (4)
-
Andrea Bolognani -
Daniel P. Berrangé -
Michal Privoznik -
Michal Prívozník