On Tue, Sep 15, 2020 at 5:53 AM Daniel P. Berrangé <berra...@redhat.com> wrote: > > On Thu, Aug 27, 2020 at 11:24:32AM -0700, William Douglas wrote:
<snip> > > +virCHMonitorPtr > > +virCHMonitorNew(virDomainObjPtr vm, const char *socketdir) > > +{ > > + virCHMonitorPtr ret = NULL; > > + virCHMonitorPtr mon = NULL; > > + virCommandPtr cmd = NULL; > > + int pings = 0; > > + > > + if (virCHMonitorInitialize() < 0) > > + return NULL; > > + > > + if (!(mon = virObjectLockableNew(virCHMonitorClass))) > > + return NULL; > > + > > + mon->socketpath = g_strdup_printf("%s/%s-socket", socketdir, > > vm->def->name); > > + > > + /* prepare to launch Cloud-Hypervisor socket */ > > + if (!(cmd = chMonitorBuildSocketCmd(vm, mon->socketpath))) > > + goto cleanup; > > + > > + if (virFileMakePath(socketdir) < 0) { > > + virReportSystemError(errno, > > + _("Cannot create socket directory '%s'"), > > + socketdir); > > + goto cleanup; > > + } > > + > > + /* launch Cloud-Hypervisor socket */ > > + if (virCommandRunAsync(cmd, &mon->pid) < 0) > > + goto cleanup; > > + > > + /* get a curl handle */ > > + mon->handle = curl_easy_init(); > > + > > + /* try to ping VMM socket 5 times to make sure it is ready */ > > + while (pings < 5) { > > + if (virCHMonitorPingVMM(mon) == 0) > > + break; > > + if (pings == 5) > > + goto cleanup; > > + > > + g_usleep(100 * 1000); > > + } > > This is highly undesirable. Is there no way to launch the CH process > such that the socket is guaranteed to be accepting requests by the > time it has forked into the background ? Or is there a way to pass > in a pre-opened FD for the monitor socket ? > > This kind of wait with timeout will cause startup failures due to > timeout under load. Currently the cloud-hypervisor process doesn't fork into the background so that was initially what I was thinking to add to cloud-hypervsior. Would tracking the pid of the cloud-hypervsior running in the background be required at that point (assuming the socket path to control the daemon would be known and working given virCommandRun returns successfully)? <snip> > What is the security model for the processes ? The libvirt driver > here is running as root and spawning CH as root. We don't really > want the VM process running as root though. It really needs to > be unprivileged from a DAC POV. Obviously that's quite a bit more > work for you todo, but it should be opssible to share alot of the > security mgmt infrastructure that we already have for QEMU and > LXC drivers. It can manage DAC permissions and apply SELinux or > AppArmor MAC labelling/profiles. Currently cloud-hypervisor runs with cap_net_admin permitted and effective on it. The implementation will be updated to run under the user's session using a non-root driver (with its own daemon as you had mentioned earlier). > > The other big question is around device addressing. If using PCI, > then PCI address assignment logic is critical to ensure consistent > guest ABI. > I'll be adding the device passthrough for the actual driver submission but if you would like to see an updated RFC with that added I could do so. > 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 :| > Thank you very much for the very thorough comments, I'll be updating my patch addressing all your feedback. William