Re: [libvirt] [PATCH 3/3] qemu: don't retry connect() if doing FD passing
On Mon, Mar 26, 2018 at 09:10:04AM -0400, John Ferlan wrote: > > > On 03/14/2018 01:33 PM, Daniel P. Berrangé wrote: > > Since libvirt called bind() and listen() on the UNIX socket, it is > > guaranteed that connect() will immediately succeed, if QEMU is running > > normally. It will only fail if QEMU has closed the monitor socket by > > mistake or if QEMU has exited, letting the kernel close it. > > > > With this in mind we can remove the retry loop and timeout when > > connecting to the QEMU monitor if we are doing FD passing. Libvirt can > > go straight to sending the QMP greeting and will simply block waiting > > for a reply until QEMU is ready. > > > > Signed-off-by: Daniel P. Berrangé> > --- > > src/qemu/qemu_capabilities.c | 2 +- > > src/qemu/qemu_monitor.c | 54 > > ++-- > > src/qemu/qemu_monitor.h | 1 + > > src/qemu/qemu_process.c | 27 -- > > tests/qemumonitortestutils.c | 1 + > > 5 files changed, 55 insertions(+), 30 deletions(-) > > > > So just doing the monitor for now... Eventually the agent too? Once we've attached to the monitor & got the QMP handshake done, we know everything else has been setup correctly. So from a functional POV there's no need to change the agent. That said I will add another a patch, since there's no reason todo the retry loop for the agent - even before FD passing, we should never have been retrying AFAICT, since we should be guaranteed that it exists at the time we connect. > How about having a news.xml patch - is this a newsworthy change? Its mostly invisible to the user I would hope > > > [...] > > > diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c > > index 57c06c7c15..9950461810 100644 > > --- a/src/qemu/qemu_process.c > > +++ b/src/qemu/qemu_process.c > > @@ -1771,7 +1771,7 @@ qemuProcessInitMonitor(virQEMUDriverPtr driver, > > > > static int > > qemuConnectMonitor(virQEMUDriverPtr driver, virDomainObjPtr vm, int > > asyncJob, > > - qemuDomainLogContextPtr logCtxt) > > + bool retry, qemuDomainLogContextPtr logCtxt) > > { > > qemuDomainObjPrivatePtr priv = vm->privateData; > > qemuMonitorPtr mon = NULL; > > @@ -1799,6 +1799,7 @@ qemuConnectMonitor(virQEMUDriverPtr driver, > > virDomainObjPtr vm, int asyncJob, > > mon = qemuMonitorOpen(vm, > >priv->monConfig, > >priv->monJSON, > > + retry, > >timeout, > >, > >driver); > > @@ -2174,17 +2175,23 @@ qemuProcessWaitForMonitor(virQEMUDriverPtr driver, > > { > > int ret = -1; > > virHashTablePtr info = NULL; > > -qemuDomainObjPrivatePtr priv; > > +qemuDomainObjPrivatePtr priv = vm->privateData; > > +bool retry = true; > > This is also called from qemuProcessAttach where it wouldn't seem > there'd be necessarily be a chardev on the command line with the > pre-opened fd, but then again since it's being used for an already > running qemu instance, that would "I hope" work properly... > > With that assumption in place, Yes, exactly - when attaching to an existing QEMU, we only want to try once and then abort, because either the socket exists, or the QEMU we're attaching to has just quit. Retrying was never right in that scenario. > > Reviewed-by: John Ferlan > > John > > > + > > +if (priv->qemuCaps && > > +virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_CHARDEV_FD_PASS)) > > +retry = false; > > > > -VIR_DEBUG("Connect monitor to %p '%s'", vm, vm->def->name); > > -if (qemuConnectMonitor(driver, vm, asyncJob, logCtxt) < 0) > > +VIR_DEBUG("Connect monitor to vm=%p name='%s' retry=%d", > > + vm, vm->def->name, retry); > > + > > +if (qemuConnectMonitor(driver, vm, asyncJob, retry, logCtxt) < 0) > > goto cleanup; > > > > /* Try to get the pty path mappings again via the monitor. This is > > much more > > * reliable if it's available. > > * Note that the monitor itself can be on a pty, so we still need to > > try the > > * log output method. */ > > -priv = vm->privateData; > > if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0) > > goto cleanup; > > ret = qemuMonitorGetChardevInfo(priv->mon, ); > > [...] > 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 :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 3/3] qemu: don't retry connect() if doing FD passing
On 03/14/2018 01:33 PM, Daniel P. Berrangé wrote: > Since libvirt called bind() and listen() on the UNIX socket, it is > guaranteed that connect() will immediately succeed, if QEMU is running > normally. It will only fail if QEMU has closed the monitor socket by > mistake or if QEMU has exited, letting the kernel close it. > > With this in mind we can remove the retry loop and timeout when > connecting to the QEMU monitor if we are doing FD passing. Libvirt can > go straight to sending the QMP greeting and will simply block waiting > for a reply until QEMU is ready. > > Signed-off-by: Daniel P. Berrangé> --- > src/qemu/qemu_capabilities.c | 2 +- > src/qemu/qemu_monitor.c | 54 > ++-- > src/qemu/qemu_monitor.h | 1 + > src/qemu/qemu_process.c | 27 -- > tests/qemumonitortestutils.c | 1 + > 5 files changed, 55 insertions(+), 30 deletions(-) > So just doing the monitor for now... Eventually the agent too? How about having a news.xml patch - is this a newsworthy change? [...] > diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c > index 57c06c7c15..9950461810 100644 > --- a/src/qemu/qemu_process.c > +++ b/src/qemu/qemu_process.c > @@ -1771,7 +1771,7 @@ qemuProcessInitMonitor(virQEMUDriverPtr driver, > > static int > qemuConnectMonitor(virQEMUDriverPtr driver, virDomainObjPtr vm, int asyncJob, > - qemuDomainLogContextPtr logCtxt) > + bool retry, qemuDomainLogContextPtr logCtxt) > { > qemuDomainObjPrivatePtr priv = vm->privateData; > qemuMonitorPtr mon = NULL; > @@ -1799,6 +1799,7 @@ qemuConnectMonitor(virQEMUDriverPtr driver, > virDomainObjPtr vm, int asyncJob, > mon = qemuMonitorOpen(vm, >priv->monConfig, >priv->monJSON, > + retry, >timeout, >, >driver); > @@ -2174,17 +2175,23 @@ qemuProcessWaitForMonitor(virQEMUDriverPtr driver, > { > int ret = -1; > virHashTablePtr info = NULL; > -qemuDomainObjPrivatePtr priv; > +qemuDomainObjPrivatePtr priv = vm->privateData; > +bool retry = true; This is also called from qemuProcessAttach where it wouldn't seem there'd be necessarily be a chardev on the command line with the pre-opened fd, but then again since it's being used for an already running qemu instance, that would "I hope" work properly... With that assumption in place, Reviewed-by: John Ferlan John > + > +if (priv->qemuCaps && > +virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_CHARDEV_FD_PASS)) > +retry = false; > > -VIR_DEBUG("Connect monitor to %p '%s'", vm, vm->def->name); > -if (qemuConnectMonitor(driver, vm, asyncJob, logCtxt) < 0) > +VIR_DEBUG("Connect monitor to vm=%p name='%s' retry=%d", > + vm, vm->def->name, retry); > + > +if (qemuConnectMonitor(driver, vm, asyncJob, retry, logCtxt) < 0) > goto cleanup; > > /* Try to get the pty path mappings again via the monitor. This is much > more > * reliable if it's available. > * Note that the monitor itself can be on a pty, so we still need to try > the > * log output method. */ > -priv = vm->privateData; > if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0) > goto cleanup; > ret = qemuMonitorGetChardevInfo(priv->mon, ); [...] -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 3/3] qemu: don't retry connect() if doing FD passing
Since libvirt called bind() and listen() on the UNIX socket, it is guaranteed that connect() will immediately succeed, if QEMU is running normally. It will only fail if QEMU has closed the monitor socket by mistake or if QEMU has exited, letting the kernel close it. With this in mind we can remove the retry loop and timeout when connecting to the QEMU monitor if we are doing FD passing. Libvirt can go straight to sending the QMP greeting and will simply block waiting for a reply until QEMU is ready. Signed-off-by: Daniel P. Berrangé--- src/qemu/qemu_capabilities.c | 2 +- src/qemu/qemu_monitor.c | 54 ++-- src/qemu/qemu_monitor.h | 1 + src/qemu/qemu_process.c | 27 -- tests/qemumonitortestutils.c | 1 + 5 files changed, 55 insertions(+), 30 deletions(-) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index c20dc32126..dd392c9c22 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -5123,7 +5123,7 @@ virQEMUCapsInitQMPCommandRun(virQEMUCapsInitQMPCommandPtr cmd, cmd->vm->pid = cmd->pid; -if (!(cmd->mon = qemuMonitorOpen(cmd->vm, >config, true, +if (!(cmd->mon = qemuMonitorOpen(cmd->vm, >config, true, true, 0, , NULL))) goto ignore; diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 1d67a97789..bf1a305656 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -349,6 +349,7 @@ qemuMonitorDispose(void *obj) static int qemuMonitorOpenUnix(const char *monitor, pid_t cpid, +bool retry, unsigned long long timeout) { struct sockaddr_un addr; @@ -370,31 +371,39 @@ qemuMonitorOpenUnix(const char *monitor, goto error; } -if (virTimeBackOffStart(, 1, timeout * 1000) < 0) -goto error; -while (virTimeBackOffWait()) { -ret = connect(monfd, (struct sockaddr *) , sizeof(addr)); - -if (ret == 0) -break; +if (retry) { +if (virTimeBackOffStart(, 1, timeout * 1000) < 0) +goto error; +while (virTimeBackOffWait()) { +ret = connect(monfd, (struct sockaddr *) , sizeof(addr)); -if ((errno == ENOENT || errno == ECONNREFUSED) && -(!cpid || virProcessKill(cpid, 0) == 0)) { -/* ENOENT : Socket may not have shown up yet - * ECONNREFUSED : Leftover socket hasn't been removed yet */ -continue; -} +if (ret == 0) +break; -virReportSystemError(errno, "%s", - _("failed to connect to monitor socket")); -goto error; +if ((errno == ENOENT || errno == ECONNREFUSED) && +(!cpid || virProcessKill(cpid, 0) == 0)) { +/* ENOENT : Socket may not have shown up yet + * ECONNREFUSED : Leftover socket hasn't been removed yet */ +continue; +} -} +virReportSystemError(errno, "%s", + _("failed to connect to monitor socket")); +goto error; +} -if (ret != 0) { -virReportSystemError(errno, "%s", - _("monitor socket did not show up")); -goto error; +if (ret != 0) { +virReportSystemError(errno, "%s", + _("monitor socket did not show up")); +goto error; +} +} else { +ret = connect(monfd, (struct sockaddr *) , sizeof(addr)); +if (ret < 0) { +virReportSystemError(errno, "%s", + _("failed to connect to monitor socket")); +goto error; +} } return monfd; @@ -914,6 +923,7 @@ qemuMonitorPtr qemuMonitorOpen(virDomainObjPtr vm, virDomainChrSourceDefPtr config, bool json, +bool retry, unsigned long long timeout, qemuMonitorCallbacksPtr cb, void *opaque) @@ -928,7 +938,7 @@ qemuMonitorOpen(virDomainObjPtr vm, case VIR_DOMAIN_CHR_TYPE_UNIX: hasSendFD = true; if ((fd = qemuMonitorOpenUnix(config->data.nix.path, - vm->pid, timeout)) < 0) + vm->pid, retry, timeout)) < 0) return NULL; break; diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index adfa87aba9..c1483c52c4 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -320,6 +320,7 @@ char *qemuMonitorUnescapeArg(const char *in); qemuMonitorPtr qemuMonitorOpen(virDomainObjPtr vm, virDomainChrSourceDefPtr config, bool json, + bool retry,