Re: [libvirt] [PATCH v2 14/23] qemu: add slirp helper unit

On 8/8/19 4:55 PM, marcandre.lureau(a)redhat.com wrote: I understand why you need this here, but it feels wrong. This must go into qemu_slirp.h and then from qemu_domain.h you include qemu_slirp.h. But that creates a circular dependency via qemu_process.h. But since you don'y really need qemuProcessIncomingDefPtr in qemu_slirp.h you only need to know if there's an incoming migration the qemuSlirpStart() argument can be turned into a boolean type and thus you qemu_process.h include can be dropped and the circular include is gone. A free function must accept NULL. Need to check if this hasn't failed. So this will run slirp-helper as root:root to query caps. I guess it's okay, as long as we don't run the actual slirp-helper that we start together with qemu as root:root. Since you're passing @stateDir as an argument, it's easy to miss that you're passing cfg->stateDir instead of cfg->slirpStateDir later in the code. So I suggest you drop @stateDir in favor of @cfg and use cfg->slirpStateDir directly. Also, @shortName can be replaced with vm->def so that we don't have to worry about getting the short name wrong. This function is prettty useless. In the only place wher it's used we already have @pidfile constructed so why not use virPidFileReadPathIfAlive() directly? Which turns out is also not correct, but I'll get to that. s/^e/ e/ The same comment applies here as for qemuSlirpCreatePidFilename(). Moreover, we are going to need the path a few more times and the prefixed version ("unix:path=...") only once. So this should be renamed to qemuSlirpGetDBusPath() and then virAsprintf() can be used again to construct the prefixed version. This grabs a reference (virObjectRef()) so in order to use returns below, this needs to be VIR_AUTOUNREF(). @id can be NULL here (we are still checking for OOM errors). Here you can use virPidFileReadPathIfAlive() directly, sicne you already have @pidfile constructed. virProcessKillPainfully() to ensure the process is really killed. This doesn't do what you think it does. I'll get to that a few lines below, but pidfile is not locked, therefore this doesn't unlink the pidfile and returns with success. Which means, we can call this directly. The dbus socket is also worth removing at this point. Ehm, s/stateDir/slirpStateDir/. But since I'm suggesting to pass @cfg directly, this change is bogus then. If we'd call qemuSlirpGetDBusPath() here, then we can avoid this string arithmetic and construct dbus_addr using an virAsprintf() call. While this works, it doesn't do what you think it does. Because of the virCommandSetPidFile() call above, the pidfile is created and written to before virCommandRun() (hidden under qemuSecurityCommandRun() here) returns. So we don't have to wait for the pidfile. This is different to how pr-helper process is spawned (which is where you get the inspiration from). The pr-helper is passed '-f $pidfile' to, which makes the pr-helper to create the pidfile (and lock it). So in that case, the pidfile might not exist upon return from virCommandRun() and thus there has to be a wait loop. Also, virCommandSetPidFile() does not cause the pidfile to be locked. This means, in the qemuSlirpStop() we have to use virPidFileReadPathIfAlive() which checks if the /proc/$pid/exe points to the expected binary to avoid us killing a different process with the same PID after PID wrap. If the pidfile was locked then we could use virPidFileForceCleanupPath() - just like pr-helper code does. What if slirp-helper dies meanwhile? We need to 'ping' it periodically here to check it's still alive (virProcessKill(pid, 0)). Also, we might set up errfd and read from it because I bet slirp-helper will produce some sensible error message on its error output just before quitting. I don't think that we can just return here. At this point, the slirp-helper is up & running so we need to kill it and remove its pid file and dbus socket. No need to include qemu_domain.h or vircommand.h. If this is turned to bool, then you can drop qemu_process.h include too. With all that addressed: Reviewed-by: Michal Privoznik <mprivozn(a)redhat.com&gt; Michal