Re: [Qemu-devel] [RFC][PATCH v2 15/17] guest agent: qemu-ga daemon
On 04/25/11 14:27, Ian Molton wrote: On Fri, 2011-04-22 at 13:51 +0200, Jes Sorensen wrote: Hiding things you miss when reading the code, it's a classic for people to do if(foo) bleh(); on the same line, and whoever reads the code will expect the action on the next line, especially if foo is a long complex statement. It's one of these 'just don't do it, it bites you in the end' things. Meh. I dont see it that way... Sure, if it was one line out of 20 written that way, it would be weird, but as is, its just part of a block of identical lines. It is a matter of consistency, we allow it in one place, we suddenly have it all over. The moment someone wants to add a slightly more complex case to such a switch statement it is all down the drain. It is way better to stay consistent across the board. I dont really see a parallel with the if() statement either since the condition in the switch() case isnt on the same line as such. I must admit that I only write one-liner if statements if the condition is short though. Writing one-liner if() statements is inherently broken, or you could call it the Perl syndrome. Write-once, read-never. Jes
Re: [Qemu-devel] [RFC][PATCH v2 15/17] guest agent: qemu-ga daemon
On Fri, 2011-04-22 at 13:51 +0200, Jes Sorensen wrote: What kind of coding error does splitting this out aim to prevent? missing break; / return; statements? Because I dont see how it achieves that... Hiding things you miss when reading the code, it's a classic for people to do if(foo) bleh(); on the same line, and whoever reads the code will expect the action on the next line, especially if foo is a long complex statement. It's one of these 'just don't do it, it bites you in the end' things. Meh. I dont see it that way... Sure, if it was one line out of 20 written that way, it would be weird, but as is, its just part of a block of identical lines. I dont really see a parallel with the if() statement either since the condition in the switch() case isnt on the same line as such. I must admit that I only write one-liner if statements if the condition is short though. -Ian
Re: [Qemu-devel] [RFC][PATCH v2 15/17] guest agent: qemu-ga daemon
On 04/21/2011 03:50 AM, Jes Sorensen wrote:
On 04/18/11 17:02, Michael Roth wrote:
+static const char *ga_log_level_str(GLogLevelFlags level)
+{
+switch (level G_LOG_LEVEL_MASK) {
+case G_LOG_LEVEL_ERROR: return error;
+case G_LOG_LEVEL_CRITICAL: return critical;
+case G_LOG_LEVEL_WARNING: return warning;
+case G_LOG_LEVEL_MESSAGE: return message;
+case G_LOG_LEVEL_INFO: return info;
+case G_LOG_LEVEL_DEBUG: return debug;
+default:return user;
+}
Urgh!
No two statements on the same line please!
Darn, I was hoping my surplus on coding style points for actually
indenting my case statements would make up for that :)
Jes
Re: [Qemu-devel] [RFC][PATCH v2 15/17] guest agent: qemu-ga daemon
On Thu, 2011-04-21 at 08:21 -0500, Michael Roth wrote:
+switch (level G_LOG_LEVEL_MASK) {
+case G_LOG_LEVEL_ERROR: return error;
+case G_LOG_LEVEL_CRITICAL: return critical;
+case G_LOG_LEVEL_WARNING: return warning;
+case G_LOG_LEVEL_MESSAGE: return message;
+case G_LOG_LEVEL_INFO: return info;
+case G_LOG_LEVEL_DEBUG: return debug;
+default:return user;
+}
Urgh!
No two statements on the same line please!
Always wondered what the logic for this one is. IMHO the above is FAR
neater than splitting it to near double its height.
What kind of coding error does splitting this out aim to prevent?
missing break; / return; statements? Because I dont see how it achieves
that...
Re: [Qemu-devel] [RFC][PATCH v2 15/17] guest agent: qemu-ga daemon
On 04/22/11 11:23, Ian Molton wrote:
On Thu, 2011-04-21 at 08:21 -0500, Michael Roth wrote:
+switch (level G_LOG_LEVEL_MASK) {
+case G_LOG_LEVEL_ERROR: return error;
+case G_LOG_LEVEL_CRITICAL: return critical;
+case G_LOG_LEVEL_WARNING: return warning;
+case G_LOG_LEVEL_MESSAGE: return message;
+case G_LOG_LEVEL_INFO: return info;
+case G_LOG_LEVEL_DEBUG: return debug;
+default:return user;
+}
Urgh!
No two statements on the same line please!
Always wondered what the logic for this one is. IMHO the above is FAR
neater than splitting it to near double its height.
What kind of coding error does splitting this out aim to prevent?
missing break; / return; statements? Because I dont see how it achieves
that...
Hiding things you miss when reading the code, it's a classic for people
to do if(foo) bleh(); on the same line, and whoever reads the code will
expect the action on the next line, especially if foo is a long complex
statement.
It's one of these 'just don't do it, it bites you in the end' things.
Jes
Re: [Qemu-devel] [RFC][PATCH v2 15/17] guest agent: qemu-ga daemon
On 04/18/11 17:02, Michael Roth wrote:
+static const char *ga_log_level_str(GLogLevelFlags level)
+{
+switch (level G_LOG_LEVEL_MASK) {
+case G_LOG_LEVEL_ERROR: return error;
+case G_LOG_LEVEL_CRITICAL: return critical;
+case G_LOG_LEVEL_WARNING: return warning;
+case G_LOG_LEVEL_MESSAGE: return message;
+case G_LOG_LEVEL_INFO: return info;
+case G_LOG_LEVEL_DEBUG: return debug;
+default:return user;
+}
Urgh!
No two statements on the same line please!
Jes
[Qemu-devel] [RFC][PATCH v2 15/17] guest agent: qemu-ga daemon
This is the actual guest daemon, it listens for requests over a
virtio-serial/isa-serial/unix socket channel and routes them through
to dispatch routines, and writes the results back to the channel in
a manner similar to QMP.
A shorthand invocation:
qemu-ga -d
Is equivalent to:
qemu-ga -c virtio-serial -p /dev/virtio-ports/org.qemu.guest_agent \
-p /var/run/qemu-guest-agent.pid -d
Signed-off-by: Michael Roth mdr...@linux.vnet.ibm.com
---
qemu-ga.c | 711 +
1 files changed, 711 insertions(+), 0 deletions(-)
create mode 100644 qemu-ga.c
diff --git a/qemu-ga.c b/qemu-ga.c
new file mode 100644
index 000..800d16b
--- /dev/null
+++ b/qemu-ga.c
@@ -0,0 +1,711 @@
+/*
+ * QEMU Guest Agent
+ *
+ * Copyright IBM Corp. 2011
+ *
+ * Authors:
+ * Adam Litkeagli...@linux.vnet.ibm.com
+ * Michael Roth mdr...@linux.vnet.ibm.com
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+#include stdlib.h
+#include stdio.h
+#include stdbool.h
+#include glib.h
+#include gio/gio.h
+#include getopt.h
+#include termios.h
+#include syslog.h
+#include qemu_socket.h
+#include json-streamer.h
+#include json-parser.h
+#include qga/guest-agent.h
+
+#define QGA_VIRTIO_PATH_DEFAULT /dev/virtio-ports/org.qemu.guest_agent
+#define QGA_PIDFILE_DEFAULT /var/run/qemu-va.pid
+#define QGA_BAUDRATE_DEFAULT B38400 /* for isa-serial channels */
+#define QGA_TIMEOUT_DEFAULT 30*1000 /* ms */
+
+struct GAState {
+bool active;
+int session_id;
+const char *proxy_path;
+JSONMessageParser parser;
+GMainLoop *main_loop;
+guint conn_id;
+GSocket *conn_sock;
+GIOChannel *conn_channel;
+guint listen_id;
+GSocket *listen_sock;
+GIOChannel *listen_channel;
+const char *path;
+const char *method;
+bool virtio; /* fastpath to check for virtio to deal with poll() quirks */
+GACommandState *command_state;
+GAWorker *worker;
+int timeout_ms;
+GLogLevelFlags log_level;
+FILE *log_file;
+};
+
+static void usage(const char *cmd)
+{
+printf(
+Usage: %s -c channel_opts\n
+QEMU Guest Agent %s\n
+\n
+ -c, --channel channel method: one of unix-connect, virtio-serial, or\n
+isa-serial (virtio-serial is the default)\n
+ -p, --pathchannel path (%s is the default for virtio-serial)\n
+ -l, --logfile set logfile path, logs to stderr by default\n
+ -f, --pidfile specify pidfile (default is %s)\n
+ -v, --verbose log extra debugging information\n
+ -V, --version print version information and exit\n
+ -d, --daemonize become a daemon\n
+ -h, --helpdisplay this help and exit\n
+\n
+Report bugs to mdr...@linux.vnet.ibm.com\n
+, cmd, QGA_VERSION, QGA_VIRTIO_PATH_DEFAULT, QGA_PIDFILE_DEFAULT);
+}
+
+static void conn_channel_close(GAState *s);
+
+static const char *ga_log_level_str(GLogLevelFlags level)
+{
+switch (level G_LOG_LEVEL_MASK) {
+case G_LOG_LEVEL_ERROR: return error;
+case G_LOG_LEVEL_CRITICAL: return critical;
+case G_LOG_LEVEL_WARNING: return warning;
+case G_LOG_LEVEL_MESSAGE: return message;
+case G_LOG_LEVEL_INFO: return info;
+case G_LOG_LEVEL_DEBUG: return debug;
+default:return user;
+}
+}
+
+static void ga_log(const gchar *domain, GLogLevelFlags level,
+ const gchar *msg, gpointer opaque)
+{
+GAState *s = opaque;
+GTimeVal time;
+const char *level_str = ga_log_level_str(level);
+
+level = G_LOG_LEVEL_MASK;
+if (g_strcmp0(domain, syslog) == 0) {
+syslog(LOG_INFO, %s: %s, level_str, msg);
+} else if (level s-log_level) {
+g_get_current_time(time);
+fprintf(s-log_file,
+%lu.%lu: %s: %s\n, time.tv_sec, time.tv_usec, level_str,
msg);
+fflush(s-log_file);
+}
+}
+
+int ga_get_timeout(GAState *s)
+{
+return s-timeout_ms;
+}
+
+static void become_daemon(const char *pidfile)
+{
+pid_t pid, sid;
+int pidfd;
+char *pidstr = NULL;
+
+pid = fork();
+if (pid 0) {
+exit(EXIT_FAILURE);
+}
+if (pid 0) {
+exit(EXIT_SUCCESS);
+}
+
+pidfd = open(pidfile, O_CREAT|O_RDWR, S_IRUSR|S_IWUSR);
+if (!pidfd || lockf(pidfd, F_TLOCK, 0)) {
+g_error(Cannot lock pid file);
+}
+
+if (ftruncate(pidfd, 0) || lseek(pidfd, 0, SEEK_SET)) {
+g_critical(Cannot truncate pid file);
+goto fail;
+}
+if (asprintf(pidstr, %d, getpid()) == -1) {
+g_critical(Cannot allocate memory);
+goto fail;
+}
+if (write(pidfd, pidstr, strlen(pidstr)) != strlen(pidstr)) {
+g_critical(Failed to write pid file);
+goto fail;
+}
+
+umask(0);
+sid = setsid();
+if (sid 0) {
+goto fail;
+}
+if ((chdir(/)) 0) {
+goto fail;
+}
