Re: [PATCH] bhyve: relax emulator binary value check

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

 



  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@xxxxxxxxx>
> > ---
> > 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@xxxxxxxxxx>
> 
> Michal
> 

Roman Bogorodskiy

Attachment: signature.asc
Description: PGP signature


[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