[libvirt] [PATCH v2] rpc: make daemon spawning a bit more intelligent
John Ferlan
jferlan at redhat.com
Tue Sep 16 14:22:53 UTC 2014
On 09/16/2014 05:16 AM, Martin Kletzander wrote:
> This way it behaves more like the daemon itself does (acquiring a
> pidfile, deleting the socket before binding, etc.).
>
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=927369
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1138604
>
> Signed-off-by: Martin Kletzander <mkletzan at redhat.com>
> ---
> src/rpc/virnetsocket.c | 65 +++++++++++++++++++++++++++++++++++++++++++-------
> 1 file changed, 57 insertions(+), 8 deletions(-)
>
The error path/retry logic needs a tweak... I added some inline thinking
since we don't have a virtual whiteboard to share on this!
> diff --git a/src/rpc/virnetsocket.c b/src/rpc/virnetsocket.c
> index 80aeddf..7be1492 100644
> --- a/src/rpc/virnetsocket.c
> +++ b/src/rpc/virnetsocket.c
> @@ -51,9 +51,11 @@
> #include "virlog.h"
> #include "virfile.h"
> #include "virthread.h"
> +#include "virpidfile.h"
> #include "virprobe.h"
> #include "virprocess.h"
> #include "virstring.h"
> +#include "dirname.h"
> #include "passfd.h"
>
> #if WITH_SSH2
> @@ -541,7 +543,10 @@ int virNetSocketNewConnectUNIX(const char *path,
> const char *binary,
> virNetSocketPtr *retsock)
> {
> + char *binname = NULL;
> + char *pidpath = NULL;
> int fd, passfd = -1;
> + int pidfd = -1;
> virSocketAddr localAddr;
> virSocketAddr remoteAddr;
>
> @@ -580,16 +585,47 @@ int virNetSocketNewConnectUNIX(const char *path,
> goto error;
> }
>
> + if (!(binname = last_component(binary)) || binname[0] == '\0') {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Cannot determine basename for binary '%s'"),
> + binary);
> + goto error;
> + }
> +
> + if (virPidFileConstructPath(false, NULL, binname, &pidpath) < 0)
> + goto error;
Since the first param is false, we are guaranteed only that 'pidpath' is
the path to the virGetUserRuntimeDirectory() for "$binname.pid".
We are not sure if we created the path in virFileMakePathHelper() or
not. If we later then delete the file on the error path how does that
affect the daemon that wins the race? See the conundrum?
> +
> + pidfd = virPidFileAcquirePath(pidpath, false, getpid());
> + VIR_FREE(pidpath);
Because you VIR_FREE() here, there is no way for the error: path to have
a non NULL pidpath... and delete the pidpath.
> + if (pidfd < 0) {
Getting here means we were unable to get the pidfile lock and I don't
think we want to delete the pidpath... Since it's probably owned by some
other daemon
> + /*
> + * This can happen in a very rare case of two clients spawning two
> + * daemons at the same time, and the error in the logs that gets
> + * reset here can be a clue to some future debugging.
> + */
> + virResetLastError();
> + spawnDaemon = false;
> + goto retry;
> + }
If we get here, we've written our pid into the pidfile and we have have
the lock... So that means we should own the file. Errors from here
should delete the file.
> +
> if ((passfd = socket(PF_UNIX, SOCK_STREAM, 0)) < 0) {
> virReportSystemError(errno, "%s", _("Failed to create socket"));
> goto error;
> }
>
> /*
> - * We have to fork() here, because umask() is set
> - * per-process, chmod() is racy and fchmod() has undefined
> - * behaviour on sockets according to POSIX, so it doesn't
> - * work outside Linux.
> + * We already even acquired the pidfile, so no one else should be using
> + * @path right now. So we're OK to unlink it and paying attention to
> + * the return value makes no real sense here. Only if it's not an
> + * abstract socket, of course.
> + */
> + if (path[0] != '@')
> + unlink(path);
> +
> + /*
> + * We have to fork() here, because umask() is set per-process, chmod()
> + * is racy and fchmod() has undefined behaviour on sockets according to
> + * POSIX, so it doesn't work outside Linux.
> */
> if ((pid = virFork()) < 0)
> goto error;
> @@ -607,12 +643,15 @@ int virNetSocketNewConnectUNIX(const char *path,
>
> if (status != EXIT_SUCCESS) {
> /*
> - * OK, so the subprocces failed to bind() the socket. This may mean
> - * that another daemon was starting at the same time and succeeded
> - * with its bind(). So we'll try connecting again, but this time
> - * without spawning the daemon.
> + * OK, so the child failed to bind() the socket. This may mean that
> + * another daemon was starting at the same time and succeeded with
> + * its bind() (even though it should not happen because we using a
> + * pidfile for the race). So we'll try connecting again, but this
> + * time without spawning the daemon.
> */
> spawnDaemon = false;
> + VIR_FORCE_CLOSE(pidfd);
And this is where it gets tricky for me with respect to whether we
should delete the file. We're supposed to have the lock on it and it
should have our pid in it. So theory says we should delete & close the
file, probably not a terrible thing to do.
Once we close, we give up the lock allowing another racer to grab and
place their pid there (and re-create if necessary since the acquire code
has logic to handle a file being deleted during a race.
If the subsequent connect fails, we have pidfd == -1 and thus the delete
won't occur in the error: path (since we no longer own the file).
If the subsequent connect passes, I assume that's like if the first one
succeeded and we wouldn't be managing the pidfile anyway.
> + VIR_FORCE_CLOSE(passfd);
> goto retry;
> }
>
> @@ -629,6 +668,12 @@ int virNetSocketNewConnectUNIX(const char *path,
> goto error;
> }
>
> + /*
> + * Do we need to eliminate the super-rare race here any more? It would
> + * need incorporating the following VIR_FORCE_CLOSE() into a
> + * virCommandHook inside a virNetSocketForkDaemon().
> + */
> + VIR_FORCE_CLOSE(pidfd);
> if (virNetSocketForkDaemon(binary, passfd) < 0)
> goto error;
> }
> @@ -645,8 +690,12 @@ int virNetSocketNewConnectUNIX(const char *path,
Making no other changes - when we get here we've either "A" closed the
pidfd and have done the ForkDaemon or "B" the connect() succeeded on
either the first or one of the retry paths (which we're not sure).
If we do the VIR_FREE(pidpath) here instead of right after AcquirePath,
then we don't leak the memory in the event we went through the connect
failure path at least once and we still allow the error path below to
delete the pidpath if it owned it.
> return 0;
>
> error:
> + if (pidfd > 0)
> + virPidFileDeletePath(pidpath);
Probably should be "if (pidfd != -1)" or ">= 0" - the corollary to "if
(pidfd < 0)"...
For the current code - pidpath == NULL, so this is a noop... Moving the
VIR_FREE(pidpath) to "later" before the return 0 allows us to delete the
pidpath which we're supposed to own on various goto error paths.
I think with moving the VIR_FREE(pidpath) to just above the return 0
will be fine. I've convinced myself we want to delete the pidpath in the
second of the retry loops, but I'm OK if it's not deleted there as well
- especially since it's not 100% clear why we're in the failure path.
And third the "pidfd != -1" should be the condition to call the delete
in the error path. I'm OK with not seeing a v3...
John
> + VIR_FREE(pidpath);
> VIR_FORCE_CLOSE(fd);
> VIR_FORCE_CLOSE(passfd);
> + VIR_FORCE_CLOSE(pidfd);
> if (spawnDaemon)
> unlink(path);
> return -1;
>
More information about the libvir-list
mailing list