Currently, requesting domain capabilities fails when the specified
emulator binary does not equal to "/usr/sbin/bhyve". Relax this check
to allow any path with basename "bhyve", as it's a common case when
binary is specified without an absolute path.

Signed-off-by: Roman Bogorodskiy <bogorodskiy@gmail.com>
---
I'm not sure how useful is this check in general: at this point we don't
use the user specified emulator value anyway, just use BHYVE constant.
Probably it would be better to just drop the entire "else" block?

 src/bhyve/bhyve_driver.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/src/bhyve/bhyve_driver.c b/src/bhyve/bhyve_driver.c
index 78c3241293..277be8dbb0 100644
--- a/src/bhyve/bhyve_driver.c
+++ b/src/bhyve/bhyve_driver.c
@@ -1647,11 +1647,14 @@ bhyveConnectGetDomainCapabilities(virConnectPtr conn,
 
     if (emulatorbin == NULL) {
         emulatorbin = "/usr/sbin/bhyve";
-    } else if (STRNEQ(emulatorbin, "/usr/sbin/bhyve")) {
-        virReportError(VIR_ERR_INVALID_ARG,
-                       _("unknown emulator binary: %s"),
-                       emulatorbin);
-        goto cleanup;
+    } else {
+        g_autofree char *emulatorbasename = g_path_get_basename(emulatorbin);
+        if (STRNEQ(emulatorbasename, "bhyve")) {
+            virReportError(VIR_ERR_INVALID_ARG,
+                           _("unknown emulator binary: %s"),
+                           emulatorbin);
+            goto cleanup;
+        }
     }
 
     if (!(caps = virBhyveDomainCapsBuild(conn->privateData, emulatorbin,
-- 
2.30.0

On 2/4/21 4:47 PM, Roman Bogorodskiy wrote:
> Currently, requesting domain capabilities fails when the specified
> emulator binary does not equal to "/usr/sbin/bhyve". Relax this check
> to allow any path with basename "bhyve", as it's a common case when
> binary is specified without an absolute path.
> 
> Signed-off-by: Roman Bogorodskiy <bogorodskiy@gmail.com>
> ---
> I'm not sure how useful is this check in general: at this point we don't
> use the user specified emulator value anyway, just use BHYVE constant.
> Probably it would be better to just drop the entire "else" block?

Yes, I was just about to suggest that. We don't do any check like this 
for QEMU driver. Just execute user provided binary (with sume arguments 
appended to get QMP monitor) and query caps.

Looking into bhyve driver code though, it doesn't use <emulator/> from 
domain XML, does it? What I'm getting at is that there is no way for a 
user to specify different binary to run than /usr/sbin/bhyve. So it 
doesn't really make sense to provide emulatorbin at all.

Since I can argue both ways, merge this patch or just remove the block 
completely.

Reviewed-by: Michal Privoznik <mprivozn@redhat.com>

Michal

  Michal Privoznik wrote:

> On 2/4/21 4:47 PM, Roman Bogorodskiy wrote:
> > Currently, requesting domain capabilities fails when the specified
> > emulator binary does not equal to "/usr/sbin/bhyve". Relax this check
> > to allow any path with basename "bhyve", as it's a common case when
> > binary is specified without an absolute path.
> > 
> > Signed-off-by: Roman Bogorodskiy <bogorodskiy@gmail.com>
> > ---
> > I'm not sure how useful is this check in general: at this point we don't
> > use the user specified emulator value anyway, just use BHYVE constant.
> > Probably it would be better to just drop the entire "else" block?
> 
> Yes, I was just about to suggest that. We don't do any check like this 
> for QEMU driver. Just execute user provided binary (with sume arguments 
> appended to get QMP monitor) and query caps.
> 
> Looking into bhyve driver code though, it doesn't use <emulator/> from 
> domain XML, does it? What I'm getting at is that there is no way for a 
> user to specify different binary to run than /usr/sbin/bhyve. So it 
> doesn't really make sense to provide emulatorbin at all.

Yes, currently we just use BHYVE (from config.h) to build commands.

In general, I've just realized that it's a little bit messy right now.
Even if we switch command line builder to use user specified value,
this will not work properly because we still use virFindFileInPath("bhyve");
to populate driver->bhyvecaps.

I guess the right way would be to move most of the code from
virBhyveProbeCaps() to virBhyveDomainCapsFill() and try to use the user
specified binary.

> Since I can argue both ways, merge this patch or just remove the block 
> completely.

I'll just remove the block then.

Thanks,

> Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
> 
> Michal
> 

Roman Bogorodskiy