[pve-devel] [RFC PATCH qemu-server] add qemumonitor.c
Wolfgang Bumiller
w.bumiller at proxmox.com
Tue Oct 9 15:57:10 CEST 2018
On Tue, Oct 09, 2018 at 02:49:22PM +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>
> ---
> sending this as rfc, without makefile/manpage/inclusion in the package/use/
> build-dependencies/etc.
>
> location and name of it are ofc subject to change :)
> i just want a general feedback of the code and the interface
>
> i had imagined starting this tool after a qemu start
> with a 'qm cleanup ID' tool to do the general cleanup
>
> the program links against libjansson4, a ~75k library with only libc6 as
> dependency, and the program uses about 100k RSS memory,
> so i think this is an acceptable overhead
> for a vm (with possibly multiple gbs of ram)
>
> qemumonitor.c | 166 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 166 insertions(+)
> create mode 100644 qemumonitor.c
>
> diff --git a/qemumonitor.c b/qemumonitor.c
> new file mode 100644
> index 0000000..13dcfa2
> --- /dev/null
> +++ b/qemumonitor.c
> @@ -0,0 +1,166 @@
> +/*
> +
> + 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 enum { false, true } bool;
Instead:
#include <stdbool.h>
> +typedef enum { STATE_PRECONNECTING, STATE_CONNECTING, STATE_CONNECTED } state_t;
> +
> +#define QMP_ANSWER "{ \"execute\":\"qmp_capabilities\" }\n"
> +
> +void usage(char *name);
make usage `static` instead
> +
> +void usage(char *name)
> +{
> + fprintf(stderr, "Usage: %s SOCKET SCRIPT [ARGUMENTS..]\n", name);
misses a dot ;-)
> +}
> +
> +int main(int argc, char *argv[])
> +{
> + if (argc < 3) {
> + usage(argv[0]);
> + exit(EXIT_FAILURE);
> + }
> +
> + ssize_t len;
Please move this to [1]
> + bool graceful_shutdown = false;
> + bool guest_requested = false;
> + bool was_reset = false;
Please move all these to [2]
> +
> + struct sockaddr_un serv_addr;
> + int sock;
> + FILE *socketfile;
It would probably be good to factor out the socket connection into a
function returning a FILE or NULL on error. Fewer variable declarations
clobbering the view.
> +
> + sock = socket(AF_UNIX, SOCK_STREAM, 0);
> + if (sock == -1) {
> + fprintf(stderr, "cannot create unix socket: %s\n", strerror(errno));
> + exit(EXIT_FAILURE);
> + }
> +
> + memset(&serv_addr, 0, sizeof(serv_addr));
> + serv_addr.sun_family = AF_UNIX;
> + memcpy(&(serv_addr.sun_path), argv[1], strlen(argv[1]));
Consider using strncpy() with sizeof(serv_addr.sun_path)-1 as size. The
last byte is already zeroed by the memset() in case you were hoping for
a strlcpy() in glibc ;-).
> +
> + 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");
> + if (socketfile == NULL) {
> + fclose(socketfile);
This should be `close(sock)` instead, as `socketfile` is NULL ;-)
> + fprintf(stderr, "error opening %s: %s\n", argv[1], strerror(errno));
> + exit(EXIT_FAILURE);
> + }
> +
> + json_t *json;
> + json_error_t err;
> + bool guest;
> + bool reset;
[2]
You can save a few extra lines by using commas:
bool guest, reset;
It's fine to list explicitly-initialized variables seprately though,
especially if the lines get too long.
> + const char *event;
> + state_t qmp_state = STATE_PRECONNECTING;
> +
> + while (!feof(socketfile)) {
> + json = json_loadf(socketfile, JSON_DISABLE_EOF_CHECK, &err);
> + if (json == NULL) {
> + // ignore parser errors
> + continue;
> + }
> + switch (qmp_state) {
> + case STATE_PRECONNECTING:
> + if (json_unpack(json, "{s: {s:{}, s:[]}}", "QMP", "version",
> + "capabilities") == 0) {
[1]
> + do {
> + len = write(sock, QMP_ANSWER, sizeof(QMP_ANSWER) - 1);
> + } while (len < 0 && errno == EINTR);
If the write fails for reasons other than pressing Ctrl+C during `qm
start` you should go into an error state and bail out instead of
entering STATE_CONNECTING, no?
> + 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 (strncmp("SHUTDOWN", event, sizeof("SHUTDOWN")) == 0) {
Are you sure this shouldn't just be a strcmp() without the 'n'? This
code would accept 'SHUTDOWNASDF', too.
(Also: Weird argument order.)
> + guest_requested = guest;
> + was_reset = reset;
> + graceful_shutdown = true;
> + }
> + }
> + break;
> + }
> + json_decref(json);
> + }
> + fclose(socketfile);
> +
> + char *script = argv[2];
> + char **args = (char **)malloc((unsigned long)(argc+2)*sizeof(char *));
Skip the pointer cast. `malloc()` returns a `void*` which does not even
require a cast when using -Weverything ;-) (it's just painful
repetition).
Also no need to repeat types in sizeof():
char **args = malloc((unsigned long)(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));
> + }
> +
> + snprintf(args[argc-2], 2, "%d", graceful_shutdown);
> + snprintf(args[argc-1], 2, "%d", guest_requested);
> + snprintf(args[argc], 2, "%d", was_reset);
> + args[argc+1] = NULL;
> + execvp(script, args);
> + exit(1);
> +}
> --
> 2.11.0
More information about the pve-devel
mailing list