[pve-devel] [PATCH qemu-server 1/4] add qmeventd
Wolfgang Bumiller
w.bumiller at proxmox.com
Wed Oct 17 11:42:58 CEST 2018
Looks good so far. Only a few minor style issues:
On Tue, Oct 16, 2018 at 12:07:03PM +0200, Dominik Csapak wrote:
> this adds a program that can listen to qemu qmp events on a given socket
> and if a shutdown event followed by a disconnected socket occurs,
> execute the given script with the given and additional arguments
>
> this is useful if we want to cleanup after the qemu process exited,
> e.g. tap devices, vgpus, etc.
>
> also we could implement a 'proper' reboot with applying pending changes
> and a stop/reboot hoook
>
> for now, this needs a not-yet applied patch[1] to qemu
> but this should be trivial to backport
>
> 1: https://lists.gnu.org/archive/html/qemu-devel/2018-10/msg01271.html
>
> Signed-off-by: Dominik Csapak <d.csapak at proxmox.com>
> ---
> Makefile | 11 ++--
> qmeventd.c | 170 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 178 insertions(+), 3 deletions(-)
> create mode 100644 qmeventd.c
>
> diff --git a/Makefile b/Makefile
> index 647edfe..22c4926 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -2,7 +2,8 @@ VERSION=5.0
> PACKAGE=qemu-server
> PKGREL=36
>
> -CFLAGS= -O2 -Werror -Wall -Wtype-limits -Wl,-z,relro
> +CFLAGS=-O2 -Werror -Wall -Wextra -Wpedantic -Wtype-limits -Wl,-z,relro
> +LIBFLAGS=$(shell pkg-config --cflags --libs jansson)
Usually you want the --cflags and --libs split up, and the --libs passed
at the end of the $(CC) invocation...
JSON_CFLAGS=...
JSON_LIBS=...
>
> DESTDIR=
> PREFIX=/usr
> @@ -36,6 +37,9 @@ all:
> dinstall: deb
> dpkg -i ${DEB}
>
> +qmeventd: qmeventd.c
> + $(CC) $(CFLAGS) ${LIBFLAGS} -o $@ $<
> +
> qm.bash-completion:
> PVE_GENERATING_DOCS=1 perl -I. -T -e "use PVE::CLI::qm; PVE::CLI::qm->generate_bash_completions();" >$@.tmp
> mv $@.tmp $@
> @@ -44,7 +48,7 @@ qmrestore.bash-completion:
> PVE_GENERATING_DOCS=1 perl -I. -T -e "use PVE::CLI::qmrestore; PVE::CLI::qmrestore->generate_bash_completions();" >$@.tmp
> mv $@.tmp $@
>
> -PKGSOURCES=qm qm.1 qmrestore qmrestore.1 qmextract qm.conf.5 qm.bash-completion qmrestore.bash-completion
> +PKGSOURCES=qm qm.1 qmrestore qmrestore.1 qmextract qm.conf.5 qm.bash-completion qmrestore.bash-completion qmeventd
>
> .PHONY: install
> install: ${PKGSOURCES}
> @@ -63,6 +67,7 @@ install: ${PKGSOURCES}
> make -C PVE install
> install -m 0755 qm ${DESTDIR}${SBINDIR}
> install -m 0755 qmrestore ${DESTDIR}${SBINDIR}
> + install -m 0755 qmeventd ${DESTDIR}${SBINDIR}
> install -m 0755 pve-bridge ${DESTDIR}${VARLIBDIR}/pve-bridge
> install -m 0755 pve-bridge-hotplug ${DESTDIR}${VARLIBDIR}/pve-bridge-hotplug
> install -m 0755 pve-bridgedown ${DESTDIR}${VARLIBDIR}/pve-bridgedown
> @@ -97,7 +102,7 @@ upload: ${DEB}
> .PHONY: clean
> clean:
> make cleanup-docgen
> - rm -rf build *.deb *.buildinfo *.changes
> + rm -rf build *.deb *.buildinfo *.changes qmeventd
> find . -name '*~' -exec rm {} ';'
>
>
> diff --git a/qmeventd.c b/qmeventd.c
> new file mode 100644
> index 0000000..f1fcc01
> --- /dev/null
> +++ b/qmeventd.c
> @@ -0,0 +1,170 @@
> +/*
> +
> + Copyright (C) 2018 Proxmox Server Solutions GmbH
> +
> + Copyright: qemumonitor is under GNU GPL, the GNU General Public License.
> +
> + This program is free software; you can redistribute it and/or modify
> + it under the terms of the GNU General Public License as published by
> + the Free Software Foundation; version 2 dated June, 1991.
> +
> + This program is distributed in the hope that it will be useful,
> + but WITHOUT ANY WARRANTY; without even the implied warranty of
> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + GNU General Public License for more details.
> +
> + You should have received a copy of the GNU General Public License
> + along with this program; if not, write to the Free Software
> + Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA
> + 02111-1307, USA.
> +
> + Author: Dominik Csapak <d.csapak at proxmox.com>
> +
> + qemumonitor connects to a given qmp socket, and waits for a
> + shutdown event followed by the closing of the socket,
> + it then calls the given script with following arguments
> +
> + SCRIPT [ARGUMENTS] <graceful> <guest> <was_reset>
> +
> + parameter explanation:
> +
> + graceful:
> + 1|0 depending if it saw a shutdown event before the socket closed
> +
> + guest:
> + 1|0 depending if the shutdown was requested from the guest
> +
> + was_reset:
> + 1|0 depending if the shutdown was actually a request
> +
> +*/
> +
> +#include <errno.h>
> +#include <sys/types.h>
> +#include <sys/socket.h>
> +#include <sys/un.h>
> +#include <unistd.h>
> +#include <stdlib.h>
> +
> +#include <jansson.h>
> +
> +typedef int json_bool_t;
> +typedef enum { STATE_PRECONNECTING, STATE_CONNECTING, STATE_CONNECTED } state_t;
> +
> +#define QMP_ANSWER "{ \"execute\":\"qmp_capabilities\" }\n"
> +
> +static void usage(char *name)
> +{
> + fprintf(stderr, "Usage: %s SOCKET SCRIPT [ARGUMENTS...]\n", name);
> +}
> +
> +int main(int argc, char *argv[])
> +{
> + if (argc < 3) {
> + usage(argv[0]);
> + exit(EXIT_FAILURE);
> + }
> +
> + struct sockaddr_un serv_addr;
> + int sock;
> + FILE *socketfile;
If you do keep the whole connect code here, I'd still prefer the
declarations as close to their uses as possible (especially since this
already happens for some other variables).
> +
> + sock = socket(AF_UNIX, SOCK_STREAM, 0);
^ int sock
> + if (sock == -1) {
> + fprintf(stderr, "cannot create unix socket: %s\n", strerror(errno));
> + exit(EXIT_FAILURE);
> + }
> +
> + size_t pathlen = strlen(argv[1]);
> +
> + if (pathlen > sizeof(serv_addr.sun_path)) {
I'd actually move this to before the socket() call. (Or for consistency
also close(sock) here as you do that in the other error cases as well
;-) .)
> + fprintf(stderr, "path is too long\n");
> + exit(EXIT_FAILURE);
> + }
> +
_ declare struct sockaddr_un serv_addr here
> + memset(&serv_addr, 0, sizeof(serv_addr));
> + serv_addr.sun_family = AF_UNIX;
> + memcpy(&(serv_addr.sun_path), argv[1], pathlen);
> +
> + if (connect(sock, (struct sockaddr *)&serv_addr, sizeof(serv_addr)) < 0) {
> + close(sock);
> + fprintf(stderr, "error connecting to %s: %s\n", argv[1], strerror(errno));
> + exit(EXIT_FAILURE);
> + }
> +
> + socketfile = fdopen(sock, "r");
^ declare FILE *socketfile here.
> + if (socketfile == NULL) {
> + close(sock);
> + fprintf(stderr, "error opening %s: %s\n", argv[1], strerror(errno));
> + exit(EXIT_FAILURE);
> + }
> +
> + json_t *json;
> + json_error_t err;
^ move 'err' into the loop
> + json_bool_t guest = 0;
> + json_bool_t reset = 0;
> + json_bool_t graceful_shutdown = 0;
> + const char *event;
^ move 'event' to STATE_CONNECTED by giving it a scope.
> + state_t qmp_state = STATE_PRECONNECTING;
> +
> + while (!feof(socketfile)) {
> + json = json_loadf(socketfile, JSON_DISABLE_EOF_CHECK, &err);
Can we pass NULL instead of err or is it mandatory? If it's mandatory,
maybe add a comment that it's not used on purpose (- or do we want a
debug mode reporting it to stderr?)
> +
> + // ignore parser errors
> + if (json == NULL)
> + continue;
> +
> + switch (qmp_state) {
> + case STATE_PRECONNECTING:
> + if (json_unpack(json, "{s: {s:{}, s:[]}}", "QMP", "version",
> + "capabilities") == 0) {
> + ssize_t len;
> + do {
> + len = write(sock, QMP_ANSWER, sizeof(QMP_ANSWER) - 1);
> + } while (len < 0 && errno == EINTR);
> + if (len != sizeof(QMP_ANSWER) - 1) {
> + fprintf(stderr, "cannot write to qmp socket: %s\n",
> + strerror(errno));
> + exit(EXIT_FAILURE);
> + }
> + qmp_state = STATE_CONNECTING;
> + }
> + break;
> + case STATE_CONNECTING:
> + if (json_unpack(json, "{s:{}}", "return") == 0) {
> + qmp_state = STATE_CONNECTED;
> + if (daemon(0, 0) == -1) {
> + fprintf(stderr, "cannot daemonize: %s\n",
> + strerror(errno));
> + exit(EXIT_FAILURE);
> + }
> + }
> + break;
> + case STATE_CONNECTED:
> + if (json_unpack(json, "{s:s, s:{s:b, s:b}}", "event", &event,
> + "data", "guest", &guest, "was_reset", &reset) == 0) {
> + if (strcmp(event, "SHUTDOWN") == 0)
> + graceful_shutdown = 1;
> + }
> + break;
> + }
> + json_decref(json);
> + }
> + fclose(socketfile);
> +
> + char *script = argv[2];
> + char **args = malloc((size_t)(argc+2) * sizeof(*args));
> +
> + for (int i = 0; i < argc - 2; i++)
> + args[i] = argv[i+2];
> +
> + for (int i = argc - 2; i < argc + 1; i++)
> + args[i] = malloc(2*sizeof(char));
^ That seems weird.
> +
> + snprintf(args[argc-2], 2, "%d", graceful_shutdown);
> + snprintf(args[argc-1], 2, "%d", guest);
> + snprintf(args[argc], 2, "%d", reset);
^ And that too.
In the comment at the top of the file you define these values to only be
allowed to be '0' or '1'. Allocating 3 2-byte buffers and using
snprintf() seems a bit overkill.
How about something like:
char shutdown_args[6] = {
graceful_shutdown ? '1' : '0', 0,
guest ? '1' : '0', 0,
reset ? '1' : '0', 0,
};
args[argc-2] = &shutdown_args[0];
args[argc-1] = &shutdown_args[2];
args[argc-0] = &shutdown_args[4];
Unless you plan on adding more values later on?
Besides, 2 of those values come directly from a json string, so the
buffers should probably be larger for those anyway.
> + args[argc+1] = NULL;
> + execvp(script, args);
> + exit(1);
> +}
> --
> 2.11.0
More information about the pve-devel
mailing list