All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2 0/7] qga: Add FreeBSD support
@ 2022-09-29  7:52 Alexander Ivanov
  2022-09-29  7:52 ` [PATCH 1/7] qga: Add initial " Alexander Ivanov
                   ` (6 more replies)
  0 siblings, 7 replies; 20+ messages in thread
From: Alexander Ivanov @ 2022-09-29  7:52 UTC (permalink / raw)
  To: qemu-devel; +Cc: den, michael.roth, kkostiuk

Add freeze/thaw, shutdown/halt/reboot and password setting support for
FreeBSD.

v2:
1: Reject the idea to move all the Linux-specific code to a separate file.
   First commit now adds initial support of FreeBSD. Fixed device paths
   and fixed virtio device initialization (disable echo). Add comment why
   we should disable the code under HAVE_GETIFADDRS in FreeBSD.
2: Replace the second commit (which now is the first) by moving
   Linux-specific freeze/thaw code to a separate file commands-linux.c.
3: Add error raising if stat() returns error. Replaced strcmp() calls by
   g_str_equal(). Add a comment explaining why UFSRESUME isn't necessary.
4: Replace #elifdef by #elif defined().
5: Now the code doesn't move from one file to aanother but still is
   moving inside file so the patch doesn't become easier to review. =(
   Fixed typos.
6,7: New patches. Add guest-network-get-interfaces command support.

Alexander Ivanov (7):
  qga: Add initial FreeBSD support
  qga: Move Linux-specific FS freeze/thaw code to a separate file
  qga: Add UFS freeze/thaw support for FreeBSD
  qga: Add shutdown/halt/reboot support for FreeBSD
  qga: Add support for user password setting in FreeBSD
  qga: Move HW address getting to a separate function
  qga: Add HW address getting for FreeBSD

 meson.build           |   2 +-
 qga/channel-posix.c   |  14 +
 qga/commands-bsd.c    | 193 ++++++++++
 qga/commands-common.h |  52 +++
 qga/commands-linux.c  | 286 ++++++++++++++
 qga/commands-posix.c  | 843 ++++++++++++++++--------------------------
 qga/main.c            |  13 +-
 qga/meson.build       |   6 +
 8 files changed, 867 insertions(+), 542 deletions(-)
 create mode 100644 qga/commands-bsd.c
 create mode 100644 qga/commands-linux.c

-- 
2.34.1



^ permalink raw reply	[flat|nested] 20+ messages in thread

* [PATCH 1/7] qga: Add initial FreeBSD support
  2022-09-29  7:52 [PATCH v2 0/7] qga: Add FreeBSD support Alexander Ivanov
@ 2022-09-29  7:52 ` Alexander Ivanov
  2022-09-29 11:29   ` Marc-André Lureau
  2022-09-29  7:52 ` [PATCH 2/7] qga: Move Linux-specific FS freeze/thaw code to a separate file Alexander Ivanov
                   ` (5 subsequent siblings)
  6 siblings, 1 reply; 20+ messages in thread
From: Alexander Ivanov @ 2022-09-29  7:52 UTC (permalink / raw)
  To: qemu-devel; +Cc: den, michael.roth, kkostiuk

- Fix device path.
- Fix virtio-serial channel initialization.
- Make the code buildable in FreeBSD.

Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
---
 meson.build          |  2 +-
 qga/channel-posix.c  | 14 ++++++++++++++
 qga/commands-posix.c |  8 ++++++++
 qga/main.c           |  6 +++++-
 4 files changed, 28 insertions(+), 2 deletions(-)

diff --git a/meson.build b/meson.build
index 8dc661363f..5c11abc8aa 100644
--- a/meson.build
+++ b/meson.build
@@ -75,7 +75,7 @@ have_tools = get_option('tools') \
   .allowed()
 have_ga = get_option('guest_agent') \
   .disable_auto_if(not have_system and not have_tools) \
-  .require(targetos in ['sunos', 'linux', 'windows'],
+  .require(targetos in ['sunos', 'linux', 'windows', 'freebsd'],
            error_message: 'unsupported OS for QEMU guest agent') \
   .allowed()
 have_block = have_system or have_tools
diff --git a/qga/channel-posix.c b/qga/channel-posix.c
index 6796a02cff..0f14246563 100644
--- a/qga/channel-posix.c
+++ b/qga/channel-posix.c
@@ -149,6 +149,20 @@ static gboolean ga_channel_open(GAChannel *c, const gchar *path,
             return false;
         }
 #endif
+#ifdef __FreeBSD__
+        struct termios tio;
+        if (tcgetattr(fd, &tio) < 0) {
+            error_setg_errno(errp, errno, "error getting channel termios attrs");
+            close(fd);
+            return false;
+        }
+        tio.c_lflag &= ~ECHO;
+        if (tcsetattr(fd, TCSAFLUSH, &tio) < 0) {
+            error_setg_errno(errp, errno, "error setting channel termios attrs");
+            close(fd);
+            return false;
+        }
+#endif /* __FreeBSD__ */
         ret = ga_channel_client_add(c, fd);
         if (ret) {
             error_setg(errp, "error adding channel to main loop");
diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index eea819cff0..16d67e9f6d 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -51,6 +51,14 @@
 #endif
 #endif
 
+#ifdef __FreeBSD__
+/*
+ * The code under HAVE_GETIFADDRS condition can't be compiled in FreeBSD.
+ * Fix it in one of the following patches.
+ */
+#undef HAVE_GETIFADDRS
+#endif
+
 #ifdef HAVE_GETIFADDRS
 #include <arpa/inet.h>
 #include <sys/socket.h>
diff --git a/qga/main.c b/qga/main.c
index 5a9d8252e0..0d27c97d38 100644
--- a/qga/main.c
+++ b/qga/main.c
@@ -45,9 +45,13 @@
 #endif
 
 #ifndef _WIN32
+#ifdef __FreeBSD__
+#define QGA_VIRTIO_PATH_DEFAULT "/dev/vtcon/org.qemu.guest_agent.0"
+#else /* __FreeBSD__ */
 #define QGA_VIRTIO_PATH_DEFAULT "/dev/virtio-ports/org.qemu.guest_agent.0"
-#define QGA_STATE_RELATIVE_DIR  "run"
+#endif /* __FreeBSD__ */
 #define QGA_SERIAL_PATH_DEFAULT "/dev/ttyS0"
+#define QGA_STATE_RELATIVE_DIR  "run"
 #else
 #define QGA_VIRTIO_PATH_DEFAULT "\\\\.\\Global\\org.qemu.guest_agent.0"
 #define QGA_STATE_RELATIVE_DIR  "qemu-ga"
-- 
2.34.1



^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH 2/7] qga: Move Linux-specific FS freeze/thaw code to a separate file
  2022-09-29  7:52 [PATCH v2 0/7] qga: Add FreeBSD support Alexander Ivanov
  2022-09-29  7:52 ` [PATCH 1/7] qga: Add initial " Alexander Ivanov
@ 2022-09-29  7:52 ` Alexander Ivanov
  2022-09-29 11:29   ` Marc-André Lureau
  2022-09-29  7:52 ` [PATCH 3/7] qga: Add UFS freeze/thaw support for FreeBSD Alexander Ivanov
                   ` (4 subsequent siblings)
  6 siblings, 1 reply; 20+ messages in thread
From: Alexander Ivanov @ 2022-09-29  7:52 UTC (permalink / raw)
  To: qemu-devel; +Cc: den, michael.roth, kkostiuk

In the next patches we are going to add FreeBSD support for QEMU Guest
Agent. In the result, code in commands-posix.c will be too cumbersome.

Move Linux-specific FS freeze/thaw code to a separate file commands-linux.c
keeping common POSIX code in commands-posix.c.

Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
---
 qga/commands-common.h |  35 +++++
 qga/commands-linux.c  | 286 +++++++++++++++++++++++++++++++++++++++++
 qga/commands-posix.c  | 289 +++---------------------------------------
 qga/meson.build       |   3 +
 4 files changed, 340 insertions(+), 273 deletions(-)
 create mode 100644 qga/commands-linux.c

diff --git a/qga/commands-common.h b/qga/commands-common.h
index d0e4a9696f..181fc330aa 100644
--- a/qga/commands-common.h
+++ b/qga/commands-common.h
@@ -10,6 +10,40 @@
 #define QGA_COMMANDS_COMMON_H
 
 #include "qga-qapi-types.h"
+#include "guest-agent-core.h"
+#include "qemu/queue.h"
+
+#if defined(__linux__)
+#include <linux/fs.h>
+#ifdef FIFREEZE
+#define CONFIG_FSFREEZE
+#endif
+#ifdef FITRIM
+#define CONFIG_FSTRIM
+#endif
+#endif /* __linux__ */
+
+#if defined(CONFIG_FSFREEZE) || defined(CONFIG_FSTRIM)
+typedef struct FsMount {
+    char *dirname;
+    char *devtype;
+    unsigned int devmajor, devminor;
+    QTAILQ_ENTRY(FsMount) next;
+} FsMount;
+
+typedef QTAILQ_HEAD(FsMountList, FsMount) FsMountList;
+
+bool build_fs_mount_list(FsMountList *mounts, Error **errp);
+void free_fs_mount_list(FsMountList *mounts);
+#endif /* CONFIG_FSFREEZE || CONFIG_FSTRIM */
+
+#if defined(CONFIG_FSFREEZE)
+int64_t qmp_guest_fsfreeze_do_freeze_list(bool has_mountpoints,
+                                          strList *mountpoints,
+                                          FsMountList mounts,
+                                          Error **errp);
+int qmp_guest_fsfreeze_do_thaw(Error **errp);
+#endif /* CONFIG_FSFREEZE */
 
 typedef struct GuestFileHandle GuestFileHandle;
 
@@ -29,4 +63,5 @@ GuestFileRead *guest_file_read_unsafe(GuestFileHandle *gfh,
  */
 char *qga_get_host_name(Error **errp);
 
+void ga_wait_child(pid_t pid, int *status, Error **errp);
 #endif
diff --git a/qga/commands-linux.c b/qga/commands-linux.c
new file mode 100644
index 0000000000..214e408fcd
--- /dev/null
+++ b/qga/commands-linux.c
@@ -0,0 +1,286 @@
+/*
+ * QEMU Guest Agent Linux-specific command implementations
+ *
+ * Copyright IBM Corp. 2011
+ *
+ * Authors:
+ *  Michael Roth      <mdroth@linux.vnet.ibm.com>
+ *  Michal Privoznik  <mprivozn@redhat.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 "qemu/osdep.h"
+#include "qapi/error.h"
+#include "commands-common.h"
+#include "cutils.h"
+#include <mntent.h>
+#include <sys/ioctl.h>
+
+#if defined(CONFIG_FSFREEZE) || defined(CONFIG_FSTRIM)
+static int dev_major_minor(const char *devpath,
+                           unsigned int *devmajor, unsigned int *devminor)
+{
+    struct stat st;
+
+    *devmajor = 0;
+    *devminor = 0;
+
+    if (stat(devpath, &st) < 0) {
+        slog("failed to stat device file '%s': %s", devpath, strerror(errno));
+        return -1;
+    }
+    if (S_ISDIR(st.st_mode)) {
+        /* It is bind mount */
+        return -2;
+    }
+    if (S_ISBLK(st.st_mode)) {
+        *devmajor = major(st.st_rdev);
+        *devminor = minor(st.st_rdev);
+        return 0;
+    }
+    return -1;
+}
+
+static bool build_fs_mount_list_from_mtab(FsMountList *mounts, Error **errp)
+{
+    struct mntent *ment;
+    FsMount *mount;
+    char const *mtab = "/proc/self/mounts";
+    FILE *fp;
+    unsigned int devmajor, devminor;
+
+    fp = setmntent(mtab, "r");
+    if (!fp) {
+        error_setg(errp, "failed to open mtab file: '%s'", mtab);
+        return false;
+    }
+
+    while ((ment = getmntent(fp))) {
+        /*
+         * An entry which device name doesn't start with a '/' is
+         * either a dummy file system or a network file system.
+         * Add special handling for smbfs and cifs as is done by
+         * coreutils as well.
+         */
+        if ((ment->mnt_fsname[0] != '/') ||
+            (strcmp(ment->mnt_type, "smbfs") == 0) ||
+            (strcmp(ment->mnt_type, "cifs") == 0)) {
+            continue;
+        }
+        if (dev_major_minor(ment->mnt_fsname, &devmajor, &devminor) == -2) {
+            /* Skip bind mounts */
+            continue;
+        }
+
+        mount = g_new0(FsMount, 1);
+        mount->dirname = g_strdup(ment->mnt_dir);
+        mount->devtype = g_strdup(ment->mnt_type);
+        mount->devmajor = devmajor;
+        mount->devminor = devminor;
+
+        QTAILQ_INSERT_TAIL(mounts, mount, next);
+    }
+
+    endmntent(fp);
+    return true;
+}
+
+static void decode_mntname(char *name, int len)
+{
+    int i, j = 0;
+    for (i = 0; i <= len; i++) {
+        if (name[i] != '\\') {
+            name[j++] = name[i];
+        } else if (name[i + 1] == '\\') {
+            name[j++] = '\\';
+            i++;
+        } else if (name[i + 1] >= '0' && name[i + 1] <= '3' &&
+                   name[i + 2] >= '0' && name[i + 2] <= '7' &&
+                   name[i + 3] >= '0' && name[i + 3] <= '7') {
+            name[j++] = (name[i + 1] - '0') * 64 +
+                        (name[i + 2] - '0') * 8 +
+                        (name[i + 3] - '0');
+            i += 3;
+        } else {
+            name[j++] = name[i];
+        }
+    }
+}
+
+/*
+ * Walk the mount table and build a list of local file systems
+ */
+bool build_fs_mount_list(FsMountList *mounts, Error **errp)
+{
+    FsMount *mount;
+    char const *mountinfo = "/proc/self/mountinfo";
+    FILE *fp;
+    char *line = NULL, *dash;
+    size_t n;
+    char check;
+    unsigned int devmajor, devminor;
+    int ret, dir_s, dir_e, type_s, type_e, dev_s, dev_e;
+
+    fp = fopen(mountinfo, "r");
+    if (!fp) {
+        return build_fs_mount_list_from_mtab(mounts, errp);
+    }
+
+    while (getline(&line, &n, fp) != -1) {
+        ret = sscanf(line, "%*u %*u %u:%u %*s %n%*s%n%c",
+                     &devmajor, &devminor, &dir_s, &dir_e, &check);
+        if (ret < 3) {
+            continue;
+        }
+        dash = strstr(line + dir_e, " - ");
+        if (!dash) {
+            continue;
+        }
+        ret = sscanf(dash, " - %n%*s%n %n%*s%n%c",
+                     &type_s, &type_e, &dev_s, &dev_e, &check);
+        if (ret < 1) {
+            continue;
+        }
+        line[dir_e] = 0;
+        dash[type_e] = 0;
+        dash[dev_e] = 0;
+        decode_mntname(line + dir_s, dir_e - dir_s);
+        decode_mntname(dash + dev_s, dev_e - dev_s);
+        if (devmajor == 0) {
+            /* btrfs reports major number = 0 */
+            if (strcmp("btrfs", dash + type_s) != 0 ||
+                dev_major_minor(dash + dev_s, &devmajor, &devminor) < 0) {
+                continue;
+            }
+        }
+
+        mount = g_new0(FsMount, 1);
+        mount->dirname = g_strdup(line + dir_s);
+        mount->devtype = g_strdup(dash + type_s);
+        mount->devmajor = devmajor;
+        mount->devminor = devminor;
+
+        QTAILQ_INSERT_TAIL(mounts, mount, next);
+    }
+    free(line);
+
+    fclose(fp);
+    return true;
+}
+#endif /* CONFIG_FSFREEZE || CONFIG_FSTRIM */
+
+#ifdef CONFIG_FSFREEZE
+/*
+ * Walk list of mounted file systems in the guest, and freeze the ones which
+ * are real local file systems.
+ */
+int64_t qmp_guest_fsfreeze_do_freeze_list(bool has_mountpoints,
+                                          strList *mountpoints,
+                                          FsMountList mounts,
+                                          Error **errp)
+{
+    struct FsMount *mount;
+    strList *list;
+    int fd, ret, i = 0;
+
+    QTAILQ_FOREACH_REVERSE(mount, &mounts, next) {
+        /* To issue fsfreeze in the reverse order of mounts, check if the
+         * mount is listed in the list here */
+        if (has_mountpoints) {
+            for (list = mountpoints; list; list = list->next) {
+                if (strcmp(list->value, mount->dirname) == 0) {
+                    break;
+                }
+            }
+            if (!list) {
+                continue;
+            }
+        }
+
+        fd = qga_open_cloexec(mount->dirname, O_RDONLY, 0);
+        if (fd == -1) {
+            error_setg_errno(errp, errno, "failed to open %s", mount->dirname);
+            return -1;
+        }
+
+        /* we try to cull filesystems we know won't work in advance, but other
+         * filesystems may not implement fsfreeze for less obvious reasons.
+         * these will report EOPNOTSUPP. we simply ignore these when tallying
+         * the number of frozen filesystems.
+         * if a filesystem is mounted more than once (aka bind mount) a
+         * consecutive attempt to freeze an already frozen filesystem will
+         * return EBUSY.
+         *
+         * any other error means a failure to freeze a filesystem we
+         * expect to be freezable, so return an error in those cases
+         * and return system to thawed state.
+         */
+        ret = ioctl(fd, FIFREEZE);
+        if (ret == -1) {
+            if (errno != EOPNOTSUPP && errno != EBUSY) {
+                error_setg_errno(errp, errno, "failed to freeze %s",
+                                 mount->dirname);
+                close(fd);
+                return -1;
+            }
+        } else {
+            i++;
+        }
+        close(fd);
+    }
+    return i;
+}
+
+int qmp_guest_fsfreeze_do_thaw(Error **errp)
+{
+    int ret;
+    FsMountList mounts;
+    FsMount *mount;
+    int fd, i = 0, logged;
+    Error *local_err = NULL;
+
+    QTAILQ_INIT(&mounts);
+    if (!build_fs_mount_list(&mounts, &local_err)) {
+        error_propagate(errp, local_err);
+        return -1;
+    }
+
+    QTAILQ_FOREACH(mount, &mounts, next) {
+        logged = false;
+        fd = qga_open_cloexec(mount->dirname, O_RDONLY, 0);
+        if (fd == -1) {
+            continue;
+        }
+        /* we have no way of knowing whether a filesystem was actually unfrozen
+         * as a result of a successful call to FITHAW, only that if an error
+         * was returned the filesystem was *not* unfrozen by that particular
+         * call.
+         *
+         * since multiple preceding FIFREEZEs require multiple calls to FITHAW
+         * to unfreeze, continuing issuing FITHAW until an error is returned,
+         * in which case either the filesystem is in an unfreezable state, or,
+         * more likely, it was thawed previously (and remains so afterward).
+         *
+         * also, since the most recent successful call is the one that did
+         * the actual unfreeze, we can use this to provide an accurate count
+         * of the number of filesystems unfrozen by guest-fsfreeze-thaw, which
+         * may * be useful for determining whether a filesystem was unfrozen
+         * during the freeze/thaw phase by a process other than qemu-ga.
+         */
+        do {
+            ret = ioctl(fd, FITHAW);
+            if (ret == 0 && !logged) {
+                i++;
+                logged = true;
+            }
+        } while (ret == 0);
+        close(fd);
+    }
+
+    free_fs_mount_list(&mounts);
+
+    return i;
+}
+#endif /* CONFIG_FSFREEZE */
diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index 16d67e9f6d..9574b83c92 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -16,11 +16,9 @@
 #include <sys/utsname.h>
 #include <sys/wait.h>
 #include <dirent.h>
-#include "guest-agent-core.h"
 #include "qga-qapi-commands.h"
 #include "qapi/error.h"
 #include "qapi/qmp/qerror.h"
-#include "qemu/queue.h"
 #include "qemu/host-utils.h"
 #include "qemu/sockets.h"
 #include "qemu/base64.h"
@@ -70,7 +68,7 @@
 #endif
 #endif
 
-static void ga_wait_child(pid_t pid, int *status, Error **errp)
+void ga_wait_child(pid_t pid, int *status, Error **errp)
 {
     pid_t rpid;
 
@@ -629,16 +627,7 @@ void qmp_guest_file_flush(int64_t handle, Error **errp)
 #if defined(__linux__)
 
 #if defined(CONFIG_FSFREEZE) || defined(CONFIG_FSTRIM)
-typedef struct FsMount {
-    char *dirname;
-    char *devtype;
-    unsigned int devmajor, devminor;
-    QTAILQ_ENTRY(FsMount) next;
-} FsMount;
-
-typedef QTAILQ_HEAD(FsMountList, FsMount) FsMountList;
-
-static void free_fs_mount_list(FsMountList *mounts)
+void free_fs_mount_list(FsMountList *mounts)
 {
      FsMount *mount, *temp;
 
@@ -653,157 +642,6 @@ static void free_fs_mount_list(FsMountList *mounts)
          g_free(mount);
      }
 }
-
-static int dev_major_minor(const char *devpath,
-                           unsigned int *devmajor, unsigned int *devminor)
-{
-    struct stat st;
-
-    *devmajor = 0;
-    *devminor = 0;
-
-    if (stat(devpath, &st) < 0) {
-        slog("failed to stat device file '%s': %s", devpath, strerror(errno));
-        return -1;
-    }
-    if (S_ISDIR(st.st_mode)) {
-        /* It is bind mount */
-        return -2;
-    }
-    if (S_ISBLK(st.st_mode)) {
-        *devmajor = major(st.st_rdev);
-        *devminor = minor(st.st_rdev);
-        return 0;
-    }
-    return -1;
-}
-
-/*
- * Walk the mount table and build a list of local file systems
- */
-static bool build_fs_mount_list_from_mtab(FsMountList *mounts, Error **errp)
-{
-    struct mntent *ment;
-    FsMount *mount;
-    char const *mtab = "/proc/self/mounts";
-    FILE *fp;
-    unsigned int devmajor, devminor;
-
-    fp = setmntent(mtab, "r");
-    if (!fp) {
-        error_setg(errp, "failed to open mtab file: '%s'", mtab);
-        return false;
-    }
-
-    while ((ment = getmntent(fp))) {
-        /*
-         * An entry which device name doesn't start with a '/' is
-         * either a dummy file system or a network file system.
-         * Add special handling for smbfs and cifs as is done by
-         * coreutils as well.
-         */
-        if ((ment->mnt_fsname[0] != '/') ||
-            (strcmp(ment->mnt_type, "smbfs") == 0) ||
-            (strcmp(ment->mnt_type, "cifs") == 0)) {
-            continue;
-        }
-        if (dev_major_minor(ment->mnt_fsname, &devmajor, &devminor) == -2) {
-            /* Skip bind mounts */
-            continue;
-        }
-
-        mount = g_new0(FsMount, 1);
-        mount->dirname = g_strdup(ment->mnt_dir);
-        mount->devtype = g_strdup(ment->mnt_type);
-        mount->devmajor = devmajor;
-        mount->devminor = devminor;
-
-        QTAILQ_INSERT_TAIL(mounts, mount, next);
-    }
-
-    endmntent(fp);
-    return true;
-}
-
-static void decode_mntname(char *name, int len)
-{
-    int i, j = 0;
-    for (i = 0; i <= len; i++) {
-        if (name[i] != '\\') {
-            name[j++] = name[i];
-        } else if (name[i + 1] == '\\') {
-            name[j++] = '\\';
-            i++;
-        } else if (name[i + 1] >= '0' && name[i + 1] <= '3' &&
-                   name[i + 2] >= '0' && name[i + 2] <= '7' &&
-                   name[i + 3] >= '0' && name[i + 3] <= '7') {
-            name[j++] = (name[i + 1] - '0') * 64 +
-                        (name[i + 2] - '0') * 8 +
-                        (name[i + 3] - '0');
-            i += 3;
-        } else {
-            name[j++] = name[i];
-        }
-    }
-}
-
-static bool build_fs_mount_list(FsMountList *mounts, Error **errp)
-{
-    FsMount *mount;
-    char const *mountinfo = "/proc/self/mountinfo";
-    FILE *fp;
-    char *line = NULL, *dash;
-    size_t n;
-    char check;
-    unsigned int devmajor, devminor;
-    int ret, dir_s, dir_e, type_s, type_e, dev_s, dev_e;
-
-    fp = fopen(mountinfo, "r");
-    if (!fp) {
-        return build_fs_mount_list_from_mtab(mounts, errp);
-    }
-
-    while (getline(&line, &n, fp) != -1) {
-        ret = sscanf(line, "%*u %*u %u:%u %*s %n%*s%n%c",
-                     &devmajor, &devminor, &dir_s, &dir_e, &check);
-        if (ret < 3) {
-            continue;
-        }
-        dash = strstr(line + dir_e, " - ");
-        if (!dash) {
-            continue;
-        }
-        ret = sscanf(dash, " - %n%*s%n %n%*s%n%c",
-                     &type_s, &type_e, &dev_s, &dev_e, &check);
-        if (ret < 1) {
-            continue;
-        }
-        line[dir_e] = 0;
-        dash[type_e] = 0;
-        dash[dev_e] = 0;
-        decode_mntname(line + dir_s, dir_e - dir_s);
-        decode_mntname(dash + dev_s, dev_e - dev_s);
-        if (devmajor == 0) {
-            /* btrfs reports major number = 0 */
-            if (strcmp("btrfs", dash + type_s) != 0 ||
-                dev_major_minor(dash + dev_s, &devmajor, &devminor) < 0) {
-                continue;
-            }
-        }
-
-        mount = g_new0(FsMount, 1);
-        mount->dirname = g_strdup(line + dir_s);
-        mount->devtype = g_strdup(dash + type_s);
-        mount->devmajor = devmajor;
-        mount->devminor = devminor;
-
-        QTAILQ_INSERT_TAIL(mounts, mount, next);
-    }
-    free(line);
-
-    fclose(fp);
-    return true;
-}
 #endif
 
 #if defined(CONFIG_FSFREEZE)
@@ -1708,20 +1546,13 @@ int64_t qmp_guest_fsfreeze_freeze(Error **errp)
     return qmp_guest_fsfreeze_freeze_list(false, NULL, errp);
 }
 
-/*
- * Walk list of mounted file systems in the guest, and freeze the ones which
- * are real local file systems.
- */
 int64_t qmp_guest_fsfreeze_freeze_list(bool has_mountpoints,
                                        strList *mountpoints,
                                        Error **errp)
 {
-    int ret = 0, i = 0;
-    strList *list;
+    int ret;
     FsMountList mounts;
-    struct FsMount *mount;
     Error *local_err = NULL;
-    int fd;
 
     slog("guest-fsfreeze called");
 
@@ -1740,122 +1571,34 @@ int64_t qmp_guest_fsfreeze_freeze_list(bool has_mountpoints,
     /* cannot risk guest agent blocking itself on a write in this state */
     ga_set_frozen(ga_state);
 
-    QTAILQ_FOREACH_REVERSE(mount, &mounts, next) {
-        /* To issue fsfreeze in the reverse order of mounts, check if the
-         * mount is listed in the list here */
-        if (has_mountpoints) {
-            for (list = mountpoints; list; list = list->next) {
-                if (strcmp(list->value, mount->dirname) == 0) {
-                    break;
-                }
-            }
-            if (!list) {
-                continue;
-            }
-        }
-
-        fd = qga_open_cloexec(mount->dirname, O_RDONLY, 0);
-        if (fd == -1) {
-            error_setg_errno(errp, errno, "failed to open %s", mount->dirname);
-            goto error;
-        }
-
-        /* we try to cull filesystems we know won't work in advance, but other
-         * filesystems may not implement fsfreeze for less obvious reasons.
-         * these will report EOPNOTSUPP. we simply ignore these when tallying
-         * the number of frozen filesystems.
-         * if a filesystem is mounted more than once (aka bind mount) a
-         * consecutive attempt to freeze an already frozen filesystem will
-         * return EBUSY.
-         *
-         * any other error means a failure to freeze a filesystem we
-         * expect to be freezable, so return an error in those cases
-         * and return system to thawed state.
-         */
-        ret = ioctl(fd, FIFREEZE);
-        if (ret == -1) {
-            if (errno != EOPNOTSUPP && errno != EBUSY) {
-                error_setg_errno(errp, errno, "failed to freeze %s",
-                                 mount->dirname);
-                close(fd);
-                goto error;
-            }
-        } else {
-            i++;
-        }
-        close(fd);
-    }
+    ret = qmp_guest_fsfreeze_do_freeze_list(has_mountpoints, mountpoints,
+                                            mounts, errp);
 
     free_fs_mount_list(&mounts);
     /* We may not issue any FIFREEZE here.
      * Just unset ga_state here and ready for the next call.
      */
-    if (i == 0) {
+    if (ret == 0) {
         ga_unset_frozen(ga_state);
+    } else if (ret < 0) {
+        qmp_guest_fsfreeze_thaw(NULL);
     }
-    return i;
-
-error:
-    free_fs_mount_list(&mounts);
-    qmp_guest_fsfreeze_thaw(NULL);
-    return 0;
+    return ret;
 }
 
-/*
- * Walk list of frozen file systems in the guest, and thaw them.
- */
 int64_t qmp_guest_fsfreeze_thaw(Error **errp)
 {
     int ret;
-    FsMountList mounts;
-    FsMount *mount;
-    int fd, i = 0, logged;
-    Error *local_err = NULL;
 
-    QTAILQ_INIT(&mounts);
-    if (!build_fs_mount_list(&mounts, &local_err)) {
-        error_propagate(errp, local_err);
-        return 0;
+    ret = qmp_guest_fsfreeze_do_thaw(errp);
+    if (ret >= 0) {
+        ga_unset_frozen(ga_state);
+        execute_fsfreeze_hook(FSFREEZE_HOOK_THAW, errp);
+    } else {
+        ret = 0;
     }
 
-    QTAILQ_FOREACH(mount, &mounts, next) {
-        logged = false;
-        fd = qga_open_cloexec(mount->dirname, O_RDONLY, 0);
-        if (fd == -1) {
-            continue;
-        }
-        /* we have no way of knowing whether a filesystem was actually unfrozen
-         * as a result of a successful call to FITHAW, only that if an error
-         * was returned the filesystem was *not* unfrozen by that particular
-         * call.
-         *
-         * since multiple preceding FIFREEZEs require multiple calls to FITHAW
-         * to unfreeze, continuing issuing FITHAW until an error is returned,
-         * in which case either the filesystem is in an unfreezable state, or,
-         * more likely, it was thawed previously (and remains so afterward).
-         *
-         * also, since the most recent successful call is the one that did
-         * the actual unfreeze, we can use this to provide an accurate count
-         * of the number of filesystems unfrozen by guest-fsfreeze-thaw, which
-         * may * be useful for determining whether a filesystem was unfrozen
-         * during the freeze/thaw phase by a process other than qemu-ga.
-         */
-        do {
-            ret = ioctl(fd, FITHAW);
-            if (ret == 0 && !logged) {
-                i++;
-                logged = true;
-            }
-        } while (ret == 0);
-        close(fd);
-    }
-
-    ga_unset_frozen(ga_state);
-    free_fs_mount_list(&mounts);
-
-    execute_fsfreeze_hook(FSFREEZE_HOOK_THAW, errp);
-
-    return i;
+    return ret;
 }
 
 static void guest_fsfreeze_cleanup(void)
diff --git a/qga/meson.build b/qga/meson.build
index 65c1e93846..409f49a000 100644
--- a/qga/meson.build
+++ b/qga/meson.build
@@ -72,6 +72,9 @@ qga_ss.add(when: 'CONFIG_POSIX', if_true: files(
   'commands-posix.c',
   'commands-posix-ssh.c',
 ))
+qga_ss.add(when: 'CONFIG_LINUX', if_true: files(
+  'commands-linux.c',
+))
 qga_ss.add(when: 'CONFIG_WIN32', if_true: files(
   'channel-win32.c',
   'commands-win32.c',
-- 
2.34.1



^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH 3/7] qga: Add UFS freeze/thaw support for FreeBSD
  2022-09-29  7:52 [PATCH v2 0/7] qga: Add FreeBSD support Alexander Ivanov
  2022-09-29  7:52 ` [PATCH 1/7] qga: Add initial " Alexander Ivanov
  2022-09-29  7:52 ` [PATCH 2/7] qga: Move Linux-specific FS freeze/thaw code to a separate file Alexander Ivanov
@ 2022-09-29  7:52 ` Alexander Ivanov
  2022-09-29 11:28   ` Marc-André Lureau
  2022-09-29  7:52 ` [PATCH 4/7] qga: Add shutdown/halt/reboot " Alexander Ivanov
                   ` (3 subsequent siblings)
  6 siblings, 1 reply; 20+ messages in thread
From: Alexander Ivanov @ 2022-09-29  7:52 UTC (permalink / raw)
  To: qemu-devel; +Cc: den, michael.roth, kkostiuk

UFS supports FS freezing through ioctl UFSSUSPEND on /dev/ufssuspend.
Frozen FS can be thawed by closing /dev/ufssuspend file descriptior.

Use getmntinfo to get a list of mounted FS.

Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
---
 qga/commands-bsd.c    | 169 +++++++++++++++++++++++
 qga/commands-common.h |  11 ++
 qga/commands-posix.c  | 308 ++++++++++++++++++++----------------------
 qga/main.c            |   7 +-
 qga/meson.build       |   3 +
 5 files changed, 334 insertions(+), 164 deletions(-)
 create mode 100644 qga/commands-bsd.c

diff --git a/qga/commands-bsd.c b/qga/commands-bsd.c
new file mode 100644
index 0000000000..ca06692179
--- /dev/null
+++ b/qga/commands-bsd.c
@@ -0,0 +1,169 @@
+/*
+ * QEMU Guest Agent BSD-specific command implementations
+ *
+ * Copyright (c) Virtuozzo International GmbH.
+ *
+ * Authors:
+ *  Alexander Ivanov  <alexander.ivanov@virtuozzo.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 "qemu/osdep.h"
+#include "qga-qapi-commands.h"
+#include "qapi/qmp/qerror.h"
+#include "qapi/error.h"
+#include "qemu/queue.h"
+#include "commands-common.h"
+#include <sys/ioctl.h>
+#include <sys/param.h>
+#include <sys/ucred.h>
+#include <sys/mount.h>
+#include <paths.h>
+
+#if defined(CONFIG_FSFREEZE) || defined(CONFIG_FSTRIM)
+bool build_fs_mount_list(FsMountList *mounts, Error **errp)
+{
+    FsMount *mount;
+    struct statfs *mntbuf, *mntp;
+    struct stat statbuf;
+    int i, count, ret;
+
+    count = getmntinfo(&mntbuf, MNT_NOWAIT);
+    if (count == 0) {
+        error_setg_errno(errp, errno, "getmntinfo failed");
+        return false;
+    }
+
+    for (i = 0; i < count; i++) {
+        mntp = &mntbuf[i];
+        ret = stat(mntp->f_mntonname, &statbuf);
+        if (ret != 0) {
+            error_setg_errno(errp, errno, "stat failed on %s",
+                             mntp->f_mntonname);
+            return false;
+        }
+
+        mount = g_new0(FsMount, 1);
+
+        mount->dirname = g_strdup(mntp->f_mntonname);
+        mount->devtype = g_strdup(mntp->f_fstypename);
+        mount->devmajor = major(mount->dev);
+        mount->devminor = minor(mount->dev);
+        mount->fsid = mntp->f_fsid;
+        mount->dev = statbuf.st_dev;
+
+        QTAILQ_INSERT_TAIL(mounts, mount, next);
+    }
+    return true;
+}
+#endif /* CONFIG_FSFREEZE || CONFIG_FSTRIM */
+
+#if defined(CONFIG_FSFREEZE)
+static int ufssuspend_fd = -1;
+static int ufssuspend_cnt;
+
+int64_t qmp_guest_fsfreeze_do_freeze_list(bool has_mountpoints,
+                                          strList *mountpoints,
+                                          FsMountList mounts,
+                                          Error **errp)
+{
+    int ret;
+    strList *list;
+    struct FsMount *mount;
+
+    if (ufssuspend_fd != -1) {
+        error_setg(errp, "filesystems have already frozen");
+        return -1;
+    }
+
+    ufssuspend_cnt = 0;
+    ufssuspend_fd = qemu_open(_PATH_UFSSUSPEND, O_RDWR, errp);
+    if (ufssuspend_fd == -1) {
+        return -1;
+    }
+
+    QTAILQ_FOREACH_REVERSE(mount, &mounts, next) {
+        /*
+         * To issue fsfreeze in the reverse order of mounts, check if the
+         * mount is listed in the list here
+         */
+        if (has_mountpoints) {
+            for (list = mountpoints; list; list = list->next) {
+                if (g_str_equal(list->value, mount->dirname)) {
+                    break;
+                }
+            }
+            if (!list) {
+                continue;
+            }
+        }
+
+        /* Only UFS supports suspend */
+        if (!g_str_equal(mount->devtype, "ufs")) {
+            continue;
+        }
+
+        ret = ioctl(ufssuspend_fd, UFSSUSPEND, &mount->fsid);
+        if (ret == -1) {
+            /*
+             * ioctl returns EBUSY for all the FS except the first one
+             * that was suspended
+             */
+            if (errno == EBUSY) {
+                continue;
+            }
+            error_setg_errno(errp, errno, "failed to freeze %s",
+                             mount->dirname);
+            goto error;
+        }
+        ufssuspend_cnt++;
+    }
+    return ufssuspend_cnt;
+error:
+    close(ufssuspend_fd);
+    ufssuspend_fd = -1;
+    return -1;
+
+}
+
+/*
+ * We don't need to call UFSRESUME ioctl because all the frozen FS
+ * are thawed on /dev/ufssuspend closing.
+ */
+int qmp_guest_fsfreeze_do_thaw(Error **errp)
+{
+    int ret = ufssuspend_cnt;
+    ufssuspend_cnt = 0;
+    if (ufssuspend_fd != -1) {
+        close(ufssuspend_fd);
+        ufssuspend_fd = -1;
+    }
+    return ret;
+}
+
+GuestFilesystemInfoList *qmp_guest_get_fsinfo(Error **errp)
+{
+    error_setg(errp, QERR_UNSUPPORTED);
+    return NULL;
+}
+
+GuestDiskInfoList *qmp_guest_get_disks(Error **errp)
+{
+    error_setg(errp, QERR_UNSUPPORTED);
+    return NULL;
+}
+
+GuestDiskStatsInfoList *qmp_guest_get_diskstats(Error **errp)
+{
+    error_setg(errp, QERR_UNSUPPORTED);
+    return NULL;
+}
+
+GuestCpuStatsList *qmp_guest_get_cpustats(Error **errp)
+{
+    error_setg(errp, QERR_UNSUPPORTED);
+    return NULL;
+}
+#endif /* CONFIG_FSFREEZE */
diff --git a/qga/commands-common.h b/qga/commands-common.h
index 181fc330aa..2d9878a634 100644
--- a/qga/commands-common.h
+++ b/qga/commands-common.h
@@ -23,11 +23,22 @@
 #endif
 #endif /* __linux__ */
 
+#ifdef __FreeBSD__
+#include <ufs/ffs/fs.h>
+#ifdef UFSSUSPEND
+#define CONFIG_FSFREEZE
+#endif
+#endif /* __FreeBSD__ */
+
 #if defined(CONFIG_FSFREEZE) || defined(CONFIG_FSTRIM)
 typedef struct FsMount {
     char *dirname;
     char *devtype;
     unsigned int devmajor, devminor;
+#if defined(__FreeBSD__)
+    dev_t dev;
+    fsid_t fsid;
+#endif
     QTAILQ_ENTRY(FsMount) next;
 } FsMount;
 
diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index 9574b83c92..49f9996a9c 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -33,20 +33,12 @@
 
 #if defined(__linux__)
 #include <mntent.h>
-#include <linux/fs.h>
 #include <sys/statvfs.h>
 #include <linux/nvme_ioctl.h>
 
 #ifdef CONFIG_LIBUDEV
 #include <libudev.h>
 #endif
-
-#ifdef FIFREEZE
-#define CONFIG_FSFREEZE
-#endif
-#ifdef FITRIM
-#define CONFIG_FSTRIM
-#endif
 #endif
 
 #ifdef __FreeBSD__
@@ -623,9 +615,6 @@ void qmp_guest_file_flush(int64_t handle, Error **errp)
     }
 }
 
-/* linux-specific implementations. avoid this if at all possible. */
-#if defined(__linux__)
-
 #if defined(CONFIG_FSFREEZE) || defined(CONFIG_FSTRIM)
 void free_fs_mount_list(FsMountList *mounts)
 {
@@ -644,6 +633,156 @@ void free_fs_mount_list(FsMountList *mounts)
 }
 #endif
 
+#if defined(CONFIG_FSFREEZE)
+typedef enum {
+    FSFREEZE_HOOK_THAW = 0,
+    FSFREEZE_HOOK_FREEZE,
+} FsfreezeHookArg;
+
+static const char *fsfreeze_hook_arg_string[] = {
+    "thaw",
+    "freeze",
+};
+
+static void execute_fsfreeze_hook(FsfreezeHookArg arg, Error **errp)
+{
+    int status;
+    pid_t pid;
+    const char *hook;
+    const char *arg_str = fsfreeze_hook_arg_string[arg];
+    Error *local_err = NULL;
+
+    hook = ga_fsfreeze_hook(ga_state);
+    if (!hook) {
+        return;
+    }
+    if (access(hook, X_OK) != 0) {
+        error_setg_errno(errp, errno, "can't access fsfreeze hook '%s'", hook);
+        return;
+    }
+
+    slog("executing fsfreeze hook with arg '%s'", arg_str);
+    pid = fork();
+    if (pid == 0) {
+        setsid();
+        reopen_fd_to_null(0);
+        reopen_fd_to_null(1);
+        reopen_fd_to_null(2);
+
+        execl(hook, hook, arg_str, NULL);
+        _exit(EXIT_FAILURE);
+    } else if (pid < 0) {
+        error_setg_errno(errp, errno, "failed to create child process");
+        return;
+    }
+
+    ga_wait_child(pid, &status, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        return;
+    }
+
+    if (!WIFEXITED(status)) {
+        error_setg(errp, "fsfreeze hook has terminated abnormally");
+        return;
+    }
+
+    status = WEXITSTATUS(status);
+    if (status) {
+        error_setg(errp, "fsfreeze hook has failed with status %d", status);
+        return;
+    }
+}
+
+/*
+ * Return status of freeze/thaw
+ */
+GuestFsfreezeStatus qmp_guest_fsfreeze_status(Error **errp)
+{
+    if (ga_is_frozen(ga_state)) {
+        return GUEST_FSFREEZE_STATUS_FROZEN;
+    }
+
+    return GUEST_FSFREEZE_STATUS_THAWED;
+}
+
+int64_t qmp_guest_fsfreeze_freeze(Error **errp)
+{
+    return qmp_guest_fsfreeze_freeze_list(false, NULL, errp);
+}
+
+int64_t qmp_guest_fsfreeze_freeze_list(bool has_mountpoints,
+                                       strList *mountpoints,
+                                       Error **errp)
+{
+    int ret;
+    FsMountList mounts;
+    Error *local_err = NULL;
+
+    slog("guest-fsfreeze called");
+
+    execute_fsfreeze_hook(FSFREEZE_HOOK_FREEZE, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        return -1;
+    }
+
+    QTAILQ_INIT(&mounts);
+    if (!build_fs_mount_list(&mounts, &local_err)) {
+        error_propagate(errp, local_err);
+        return -1;
+    }
+
+    /* cannot risk guest agent blocking itself on a write in this state */
+    ga_set_frozen(ga_state);
+
+    ret = qmp_guest_fsfreeze_do_freeze_list(has_mountpoints, mountpoints,
+                                            mounts, errp);
+
+    free_fs_mount_list(&mounts);
+    /* We may not issue any FIFREEZE here.
+     * Just unset ga_state here and ready for the next call.
+     */
+    if (ret == 0) {
+        ga_unset_frozen(ga_state);
+    } else if (ret < 0) {
+        qmp_guest_fsfreeze_thaw(NULL);
+    }
+    return ret;
+}
+
+int64_t qmp_guest_fsfreeze_thaw(Error **errp)
+{
+    int ret;
+
+    ret = qmp_guest_fsfreeze_do_thaw(errp);
+    if (ret >= 0) {
+        ga_unset_frozen(ga_state);
+        execute_fsfreeze_hook(FSFREEZE_HOOK_THAW, errp);
+    } else {
+        ret = 0;
+    }
+
+    return ret;
+}
+
+static void guest_fsfreeze_cleanup(void)
+{
+    Error *err = NULL;
+
+    if (ga_is_frozen(ga_state) == GUEST_FSFREEZE_STATUS_FROZEN) {
+        qmp_guest_fsfreeze_thaw(&err);
+        if (err) {
+            slog("failed to clean up frozen filesystems: %s",
+                 error_get_pretty(err));
+            error_free(err);
+        }
+    }
+}
+#endif
+
+/* linux-specific implementations. avoid this if at all possible. */
+#if defined(__linux__)
 #if defined(CONFIG_FSFREEZE)
 
 static char *get_pci_driver(char const *syspath, int pathlen, Error **errp)
@@ -1467,153 +1606,6 @@ GuestFilesystemInfoList *qmp_guest_get_fsinfo(Error **errp)
     free_fs_mount_list(&mounts);
     return ret;
 }
-
-
-typedef enum {
-    FSFREEZE_HOOK_THAW = 0,
-    FSFREEZE_HOOK_FREEZE,
-} FsfreezeHookArg;
-
-static const char *fsfreeze_hook_arg_string[] = {
-    "thaw",
-    "freeze",
-};
-
-static void execute_fsfreeze_hook(FsfreezeHookArg arg, Error **errp)
-{
-    int status;
-    pid_t pid;
-    const char *hook;
-    const char *arg_str = fsfreeze_hook_arg_string[arg];
-    Error *local_err = NULL;
-
-    hook = ga_fsfreeze_hook(ga_state);
-    if (!hook) {
-        return;
-    }
-    if (access(hook, X_OK) != 0) {
-        error_setg_errno(errp, errno, "can't access fsfreeze hook '%s'", hook);
-        return;
-    }
-
-    slog("executing fsfreeze hook with arg '%s'", arg_str);
-    pid = fork();
-    if (pid == 0) {
-        setsid();
-        reopen_fd_to_null(0);
-        reopen_fd_to_null(1);
-        reopen_fd_to_null(2);
-
-        execl(hook, hook, arg_str, NULL);
-        _exit(EXIT_FAILURE);
-    } else if (pid < 0) {
-        error_setg_errno(errp, errno, "failed to create child process");
-        return;
-    }
-
-    ga_wait_child(pid, &status, &local_err);
-    if (local_err) {
-        error_propagate(errp, local_err);
-        return;
-    }
-
-    if (!WIFEXITED(status)) {
-        error_setg(errp, "fsfreeze hook has terminated abnormally");
-        return;
-    }
-
-    status = WEXITSTATUS(status);
-    if (status) {
-        error_setg(errp, "fsfreeze hook has failed with status %d", status);
-        return;
-    }
-}
-
-/*
- * Return status of freeze/thaw
- */
-GuestFsfreezeStatus qmp_guest_fsfreeze_status(Error **errp)
-{
-    if (ga_is_frozen(ga_state)) {
-        return GUEST_FSFREEZE_STATUS_FROZEN;
-    }
-
-    return GUEST_FSFREEZE_STATUS_THAWED;
-}
-
-int64_t qmp_guest_fsfreeze_freeze(Error **errp)
-{
-    return qmp_guest_fsfreeze_freeze_list(false, NULL, errp);
-}
-
-int64_t qmp_guest_fsfreeze_freeze_list(bool has_mountpoints,
-                                       strList *mountpoints,
-                                       Error **errp)
-{
-    int ret;
-    FsMountList mounts;
-    Error *local_err = NULL;
-
-    slog("guest-fsfreeze called");
-
-    execute_fsfreeze_hook(FSFREEZE_HOOK_FREEZE, &local_err);
-    if (local_err) {
-        error_propagate(errp, local_err);
-        return -1;
-    }
-
-    QTAILQ_INIT(&mounts);
-    if (!build_fs_mount_list(&mounts, &local_err)) {
-        error_propagate(errp, local_err);
-        return -1;
-    }
-
-    /* cannot risk guest agent blocking itself on a write in this state */
-    ga_set_frozen(ga_state);
-
-    ret = qmp_guest_fsfreeze_do_freeze_list(has_mountpoints, mountpoints,
-                                            mounts, errp);
-
-    free_fs_mount_list(&mounts);
-    /* We may not issue any FIFREEZE here.
-     * Just unset ga_state here and ready for the next call.
-     */
-    if (ret == 0) {
-        ga_unset_frozen(ga_state);
-    } else if (ret < 0) {
-        qmp_guest_fsfreeze_thaw(NULL);
-    }
-    return ret;
-}
-
-int64_t qmp_guest_fsfreeze_thaw(Error **errp)
-{
-    int ret;
-
-    ret = qmp_guest_fsfreeze_do_thaw(errp);
-    if (ret >= 0) {
-        ga_unset_frozen(ga_state);
-        execute_fsfreeze_hook(FSFREEZE_HOOK_THAW, errp);
-    } else {
-        ret = 0;
-    }
-
-    return ret;
-}
-
-static void guest_fsfreeze_cleanup(void)
-{
-    Error *err = NULL;
-
-    if (ga_is_frozen(ga_state) == GUEST_FSFREEZE_STATUS_FROZEN) {
-        qmp_guest_fsfreeze_thaw(&err);
-        if (err) {
-            slog("failed to clean up frozen filesystems: %s",
-                 error_get_pretty(err));
-            error_free(err);
-        }
-    }
-}
 #endif /* CONFIG_FSFREEZE */
 
 #if defined(CONFIG_FSTRIM)
diff --git a/qga/main.c b/qga/main.c
index 0d27c97d38..b3580508fa 100644
--- a/qga/main.c
+++ b/qga/main.c
@@ -37,12 +37,7 @@
 #include "qga/service-win32.h"
 #include "qga/vss-win32.h"
 #endif
-#ifdef __linux__
-#include <linux/fs.h>
-#ifdef FIFREEZE
-#define CONFIG_FSFREEZE
-#endif
-#endif
+#include "commands-common.h"
 
 #ifndef _WIN32
 #ifdef __FreeBSD__
diff --git a/qga/meson.build b/qga/meson.build
index 409f49a000..456ba4c29f 100644
--- a/qga/meson.build
+++ b/qga/meson.build
@@ -75,6 +75,9 @@ qga_ss.add(when: 'CONFIG_POSIX', if_true: files(
 qga_ss.add(when: 'CONFIG_LINUX', if_true: files(
   'commands-linux.c',
 ))
+qga_ss.add(when: 'CONFIG_BSD', if_true: files(
+  'commands-bsd.c',
+))
 qga_ss.add(when: 'CONFIG_WIN32', if_true: files(
   'channel-win32.c',
   'commands-win32.c',
-- 
2.34.1



^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH 4/7] qga: Add shutdown/halt/reboot support for FreeBSD
  2022-09-29  7:52 [PATCH v2 0/7] qga: Add FreeBSD support Alexander Ivanov
                   ` (2 preceding siblings ...)
  2022-09-29  7:52 ` [PATCH 3/7] qga: Add UFS freeze/thaw support for FreeBSD Alexander Ivanov
@ 2022-09-29  7:52 ` Alexander Ivanov
  2022-09-29 11:28   ` Marc-André Lureau
  2022-09-29  7:52 ` [PATCH 5/7] qga: Add support for user password setting in FreeBSD Alexander Ivanov
                   ` (2 subsequent siblings)
  6 siblings, 1 reply; 20+ messages in thread
From: Alexander Ivanov @ 2022-09-29  7:52 UTC (permalink / raw)
  To: qemu-devel; +Cc: den, michael.roth, kkostiuk

Add appropriate shutdown command arguments to qmp_guest_shutdown()
for FreeBSD.

Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
---
 qga/commands-posix.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index 49f9996a9c..88e0d0fe24 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -90,6 +90,10 @@ void qmp_guest_shutdown(bool has_mode, const char *mode, Error **errp)
     const char *powerdown_flag = "-i5";
     const char *halt_flag = "-i0";
     const char *reboot_flag = "-i6";
+#elif defined(CONFIG_BSD)
+    const char *powerdown_flag = "-p";
+    const char *halt_flag = "-h";
+    const char *reboot_flag = "-r";
 #else
     const char *powerdown_flag = "-P";
     const char *halt_flag = "-H";
@@ -120,6 +124,9 @@ void qmp_guest_shutdown(bool has_mode, const char *mode, Error **errp)
 #ifdef CONFIG_SOLARIS
         execl("/sbin/shutdown", "shutdown", shutdown_flag, "-g0", "-y",
               "hypervisor initiated shutdown", (char *)NULL);
+#elif defined(CONFIG_BSD)
+        execl("/sbin/shutdown", "shutdown", shutdown_flag, "+0",
+               "hypervisor initiated shutdown", (char *)NULL);
 #else
         execl("/sbin/shutdown", "shutdown", "-h", shutdown_flag, "+0",
                "hypervisor initiated shutdown", (char *)NULL);
-- 
2.34.1



^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH 5/7] qga: Add support for user password setting in FreeBSD
  2022-09-29  7:52 [PATCH v2 0/7] qga: Add FreeBSD support Alexander Ivanov
                   ` (3 preceding siblings ...)
  2022-09-29  7:52 ` [PATCH 4/7] qga: Add shutdown/halt/reboot " Alexander Ivanov
@ 2022-09-29  7:52 ` Alexander Ivanov
  2022-09-29 11:22   ` Marc-André Lureau
  2022-09-29  7:52 ` [PATCH 6/7] qga: Move HW address getting to a separate function Alexander Ivanov
  2022-09-29  7:52 ` [PATCH 7/7] qga: Add HW address getting for FreeBSD Alexander Ivanov
  6 siblings, 1 reply; 20+ messages in thread
From: Alexander Ivanov @ 2022-09-29  7:52 UTC (permalink / raw)
  To: qemu-devel; +Cc: den, michael.roth, kkostiuk

Move qmp_guest_set_user_password() from __linux__ condition to
(__linux__ || __FreeBSD__) condition. Add command and arguments
for password setting in FreeBSD.

Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
---
 qga/commands-posix.c | 223 +++++++++++++++++++++++--------------------
 1 file changed, 118 insertions(+), 105 deletions(-)

diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index 88e0d0fe24..6ce894ca6e 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -2123,111 +2123,6 @@ int64_t qmp_guest_set_vcpus(GuestLogicalProcessorList *vcpus, Error **errp)
     return processed;
 }
 
-void qmp_guest_set_user_password(const char *username,
-                                 const char *password,
-                                 bool crypted,
-                                 Error **errp)
-{
-    Error *local_err = NULL;
-    char *passwd_path = NULL;
-    pid_t pid;
-    int status;
-    int datafd[2] = { -1, -1 };
-    char *rawpasswddata = NULL;
-    size_t rawpasswdlen;
-    char *chpasswddata = NULL;
-    size_t chpasswdlen;
-
-    rawpasswddata = (char *)qbase64_decode(password, -1, &rawpasswdlen, errp);
-    if (!rawpasswddata) {
-        return;
-    }
-    rawpasswddata = g_renew(char, rawpasswddata, rawpasswdlen + 1);
-    rawpasswddata[rawpasswdlen] = '\0';
-
-    if (strchr(rawpasswddata, '\n')) {
-        error_setg(errp, "forbidden characters in raw password");
-        goto out;
-    }
-
-    if (strchr(username, '\n') ||
-        strchr(username, ':')) {
-        error_setg(errp, "forbidden characters in username");
-        goto out;
-    }
-
-    chpasswddata = g_strdup_printf("%s:%s\n", username, rawpasswddata);
-    chpasswdlen = strlen(chpasswddata);
-
-    passwd_path = g_find_program_in_path("chpasswd");
-
-    if (!passwd_path) {
-        error_setg(errp, "cannot find 'passwd' program in PATH");
-        goto out;
-    }
-
-    if (!g_unix_open_pipe(datafd, FD_CLOEXEC, NULL)) {
-        error_setg(errp, "cannot create pipe FDs");
-        goto out;
-    }
-
-    pid = fork();
-    if (pid == 0) {
-        close(datafd[1]);
-        /* child */
-        setsid();
-        dup2(datafd[0], 0);
-        reopen_fd_to_null(1);
-        reopen_fd_to_null(2);
-
-        if (crypted) {
-            execl(passwd_path, "chpasswd", "-e", NULL);
-        } else {
-            execl(passwd_path, "chpasswd", NULL);
-        }
-        _exit(EXIT_FAILURE);
-    } else if (pid < 0) {
-        error_setg_errno(errp, errno, "failed to create child process");
-        goto out;
-    }
-    close(datafd[0]);
-    datafd[0] = -1;
-
-    if (qemu_write_full(datafd[1], chpasswddata, chpasswdlen) != chpasswdlen) {
-        error_setg_errno(errp, errno, "cannot write new account password");
-        goto out;
-    }
-    close(datafd[1]);
-    datafd[1] = -1;
-
-    ga_wait_child(pid, &status, &local_err);
-    if (local_err) {
-        error_propagate(errp, local_err);
-        goto out;
-    }
-
-    if (!WIFEXITED(status)) {
-        error_setg(errp, "child process has terminated abnormally");
-        goto out;
-    }
-
-    if (WEXITSTATUS(status)) {
-        error_setg(errp, "child process has failed to set user password");
-        goto out;
-    }
-
-out:
-    g_free(chpasswddata);
-    g_free(rawpasswddata);
-    g_free(passwd_path);
-    if (datafd[0] != -1) {
-        close(datafd[0]);
-    }
-    if (datafd[1] != -1) {
-        close(datafd[1]);
-    }
-}
-
 static void ga_read_sysfs_file(int dirfd, const char *pathname, char *buf,
                                int size, Error **errp)
 {
@@ -2793,6 +2688,124 @@ GuestMemoryBlockInfo *qmp_guest_get_memory_block_info(Error **errp)
 
 #endif
 
+#if defined(__linux__) || defined(__FreeBSD__)
+void qmp_guest_set_user_password(const char *username,
+                                 const char *password,
+                                 bool crypted,
+                                 Error **errp)
+{
+    Error *local_err = NULL;
+    char *passwd_path = NULL;
+    pid_t pid;
+    int status;
+    int datafd[2] = { -1, -1 };
+    char *rawpasswddata = NULL;
+    size_t rawpasswdlen;
+    char *chpasswddata = NULL;
+    size_t chpasswdlen;
+
+    rawpasswddata = (char *)qbase64_decode(password, -1, &rawpasswdlen, errp);
+    if (!rawpasswddata) {
+        return;
+    }
+    rawpasswddata = g_renew(char, rawpasswddata, rawpasswdlen + 1);
+    rawpasswddata[rawpasswdlen] = '\0';
+
+    if (strchr(rawpasswddata, '\n')) {
+        error_setg(errp, "forbidden characters in raw password");
+        goto out;
+    }
+
+    if (strchr(username, '\n') ||
+        strchr(username, ':')) {
+        error_setg(errp, "forbidden characters in username");
+        goto out;
+    }
+
+#ifdef __FreeBSD__
+    chpasswddata = g_strdup(rawpasswddata);
+    passwd_path = g_find_program_in_path("pw");
+#else
+    chpasswddata = g_strdup_printf("%s:%s\n", username, rawpasswddata);
+    passwd_path = g_find_program_in_path("chpasswd");
+#endif
+
+    chpasswdlen = strlen(chpasswddata);
+
+    if (!passwd_path) {
+        error_setg(errp, "cannot find 'passwd' program in PATH");
+        goto out;
+    }
+
+    if (!g_unix_open_pipe(datafd, FD_CLOEXEC, NULL)) {
+        error_setg(errp, "cannot create pipe FDs");
+        goto out;
+    }
+
+    pid = fork();
+    if (pid == 0) {
+        close(datafd[1]);
+        /* child */
+        setsid();
+        dup2(datafd[0], 0);
+        reopen_fd_to_null(1);
+        reopen_fd_to_null(2);
+
+#ifdef __FreeBSD__
+        const char *h_arg;
+        h_arg = (crypted) ? "-H" : "-h";
+        execl(passwd_path, "pw", "usermod", "-n", username, h_arg, "0", NULL);
+#else
+        if (crypted) {
+            execl(passwd_path, "chpasswd", "-e", NULL);
+        } else {
+            execl(passwd_path, "chpasswd", NULL);
+        }
+#endif
+        _exit(EXIT_FAILURE);
+    } else if (pid < 0) {
+        error_setg_errno(errp, errno, "failed to create child process");
+        goto out;
+    }
+    close(datafd[0]);
+    datafd[0] = -1;
+
+    if (qemu_write_full(datafd[1], chpasswddata, chpasswdlen) != chpasswdlen) {
+        error_setg_errno(errp, errno, "cannot write new account password");
+        goto out;
+    }
+    close(datafd[1]);
+    datafd[1] = -1;
+
+    ga_wait_child(pid, &status, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        goto out;
+    }
+
+    if (!WIFEXITED(status)) {
+        error_setg(errp, "child process has terminated abnormally");
+        goto out;
+    }
+
+    if (WEXITSTATUS(status)) {
+        error_setg(errp, "child process has failed to set user password");
+        goto out;
+    }
+
+out:
+    g_free(chpasswddata);
+    g_free(rawpasswddata);
+    g_free(passwd_path);
+    if (datafd[0] != -1) {
+        close(datafd[0]);
+    }
+    if (datafd[1] != -1) {
+        close(datafd[1]);
+    }
+}
+#endif
+
 #ifdef HAVE_GETIFADDRS
 static GuestNetworkInterface *
 guest_find_interface(GuestNetworkInterfaceList *head,
-- 
2.34.1



^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH 6/7] qga: Move HW address getting to a separate function
  2022-09-29  7:52 [PATCH v2 0/7] qga: Add FreeBSD support Alexander Ivanov
                   ` (4 preceding siblings ...)
  2022-09-29  7:52 ` [PATCH 5/7] qga: Add support for user password setting in FreeBSD Alexander Ivanov
@ 2022-09-29  7:52 ` Alexander Ivanov
  2022-09-29 11:28   ` Marc-André Lureau
  2022-09-29  7:52 ` [PATCH 7/7] qga: Add HW address getting for FreeBSD Alexander Ivanov
  6 siblings, 1 reply; 20+ messages in thread
From: Alexander Ivanov @ 2022-09-29  7:52 UTC (permalink / raw)
  To: qemu-devel; +Cc: den, michael.roth, kkostiuk

In the next patch FreeBSD support for guest-network-get-interfaces will be
added. Previously move Linux-specific code of HW address getting to a
separate functions and add a dumb function to commands-bsd.c.

Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
---
 qga/commands-bsd.c    |  18 +++++++
 qga/commands-common.h |   6 +++
 qga/commands-posix.c  | 114 +++++++++++++++++++++++-------------------
 3 files changed, 87 insertions(+), 51 deletions(-)

diff --git a/qga/commands-bsd.c b/qga/commands-bsd.c
index ca06692179..ca81114171 100644
--- a/qga/commands-bsd.c
+++ b/qga/commands-bsd.c
@@ -167,3 +167,21 @@ GuestCpuStatsList *qmp_guest_get_cpustats(Error **errp)
     return NULL;
 }
 #endif /* CONFIG_FSFREEZE */
+
+#ifdef HAVE_GETIFADDRS
+/*
+ * Fill buf with MAC address by ifaddrs. Pointer buf must point to a
+ * buffer with ETHER_ADDR_LEN length at least.
+ *
+ * Returns -1 in case of an error, 0 if MAC address can't be obtained or
+ * 1 if MAC addres is obtained.
+ */
+int guest_get_hw_addr(struct ifaddrs *ifa, unsigned char *buf, Error **errp)
+{
+    /*
+     * We can't get the hw addr of this interface,
+     * but that's not a fatal error.
+     */
+    return 0;
+}
+#endif /* HAVE_GETIFADDRS */
diff --git a/qga/commands-common.h b/qga/commands-common.h
index 2d9878a634..2485a037fd 100644
--- a/qga/commands-common.h
+++ b/qga/commands-common.h
@@ -56,6 +56,12 @@ int64_t qmp_guest_fsfreeze_do_freeze_list(bool has_mountpoints,
 int qmp_guest_fsfreeze_do_thaw(Error **errp);
 #endif /* CONFIG_FSFREEZE */
 
+#ifdef HAVE_GETIFADDRS
+#include <ifaddrs.h>
+
+int guest_get_hw_addr(struct ifaddrs *ifa, unsigned char *buf, Error **errp);
+#endif
+
 typedef struct GuestFileHandle GuestFileHandle;
 
 GuestFileHandle *guest_file_handle_find(int64_t id, Error **errp);
diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index 6ce894ca6e..0bebd9e690 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -41,20 +41,12 @@
 #endif
 #endif
 
-#ifdef __FreeBSD__
-/*
- * The code under HAVE_GETIFADDRS condition can't be compiled in FreeBSD.
- * Fix it in one of the following patches.
- */
-#undef HAVE_GETIFADDRS
-#endif
-
 #ifdef HAVE_GETIFADDRS
 #include <arpa/inet.h>
 #include <sys/socket.h>
 #include <net/if.h>
+#include <net/ethernet.h>
 #include <sys/types.h>
-#include <ifaddrs.h>
 #ifdef CONFIG_SOLARIS
 #include <sys/sockio.h>
 #endif
@@ -2659,14 +2651,6 @@ int64_t qmp_guest_set_vcpus(GuestLogicalProcessorList *vcpus, Error **errp)
     return -1;
 }
 
-void qmp_guest_set_user_password(const char *username,
-                                 const char *password,
-                                 bool crypted,
-                                 Error **errp)
-{
-    error_setg(errp, QERR_UNSUPPORTED);
-}
-
 GuestMemoryBlockList *qmp_guest_get_memory_blocks(Error **errp)
 {
     error_setg(errp, QERR_UNSUPPORTED);
@@ -2804,7 +2788,15 @@ out:
         close(datafd[1]);
     }
 }
-#endif
+#else /* __linux__ || __FreeBSD__ */
+void qmp_guest_set_user_password(const char *username,
+                                 const char *password,
+                                 bool crypted,
+                                 Error **errp)
+{
+    error_setg(errp, QERR_UNSUPPORTED);
+}
+#endif /* __linux__ || __FreeBSD__ */
 
 #ifdef HAVE_GETIFADDRS
 static GuestNetworkInterface *
@@ -2887,6 +2879,54 @@ static int guest_get_network_stats(const char *name,
     return -1;
 }
 
+#ifndef __FreeBSD__
+/*
+ * Fill buf with MAC address by ifaddrs. Pointer buf must point to a
+ * buffer with ETHER_ADDR_LEN length at least.
+ * Returns -1 in case of an error, 0 if MAC address can't be obtained or
+ * 1 if MAC addres is obtained.
+ */
+int guest_get_hw_addr(struct ifaddrs *ifa, unsigned char *buf, Error **errp)
+{
+    struct ifreq ifr;
+    int sock;
+
+    /* we haven't obtained HW address yet */
+    sock = socket(PF_INET, SOCK_STREAM, 0);
+    if (sock == -1) {
+        error_setg_errno(errp, errno, "failed to create socket");
+        return -1;
+    }
+
+    memset(&ifr, 0, sizeof(ifr));
+    pstrcpy(ifr.ifr_name, IF_NAMESIZE, ifa->ifa_name);
+    if (ioctl(sock, SIOCGIFHWADDR, &ifr) == -1) {
+        /*
+         * We can't get the hw addr of this interface, but that's not a
+         * fatal error. Don't set info->hardware_address, but keep
+         * going.
+         */
+        if (errno == EADDRNOTAVAIL) {
+            /* The interface doesn't have a hw addr (e.g. loopback). */
+            g_debug("failed to get MAC address of %s: %s",
+                    ifa->ifa_name, strerror(errno));
+        } else{
+            g_warning("failed to get MAC address of %s: %s",
+                      ifa->ifa_name, strerror(errno));
+        }
+        close(sock);
+        return 0;
+    }
+#ifdef CONFIG_SOLARIS
+    memcpy(buf, &ifr.ifr_addr.sa_data, ETHER_ADDR_LEN);
+#else
+    memcpy(buf, &ifr.ifr_hwaddr.sa_data, ETHER_ADDR_LEN);
+#endif
+    close(sock);
+    return 1;
+}
+#endif /* __FreeBSD__ */
+
 /*
  * Build information about guest interfaces
  */
@@ -2907,10 +2947,9 @@ GuestNetworkInterfaceList *qmp_guest_network_get_interfaces(Error **errp)
         GuestNetworkInterfaceStat *interface_stat = NULL;
         char addr4[INET_ADDRSTRLEN];
         char addr6[INET6_ADDRSTRLEN];
-        int sock;
-        struct ifreq ifr;
-        unsigned char *mac_addr;
+        unsigned char mac_addr[ETHER_ADDR_LEN];
         void *p;
+        int ret;
 
         g_debug("Processing %s interface", ifa->ifa_name);
 
@@ -2924,45 +2963,18 @@ GuestNetworkInterfaceList *qmp_guest_network_get_interfaces(Error **errp)
         }
 
         if (!info->has_hardware_address) {
-            /* we haven't obtained HW address yet */
-            sock = socket(PF_INET, SOCK_STREAM, 0);
-            if (sock == -1) {
-                error_setg_errno(errp, errno, "failed to create socket");
+            ret = guest_get_hw_addr(ifa, mac_addr, errp);
+            if (ret == -1) {
                 goto error;
             }
-
-            memset(&ifr, 0, sizeof(ifr));
-            pstrcpy(ifr.ifr_name, IF_NAMESIZE, info->name);
-            if (ioctl(sock, SIOCGIFHWADDR, &ifr) == -1) {
-                /*
-                 * We can't get the hw addr of this interface, but that's not a
-                 * fatal error. Don't set info->hardware_address, but keep
-                 * going.
-                 */
-                if (errno == EADDRNOTAVAIL) {
-                    /* The interface doesn't have a hw addr (e.g. loopback). */
-                    g_debug("failed to get MAC address of %s: %s",
-                            ifa->ifa_name, strerror(errno));
-                } else{
-                    g_warning("failed to get MAC address of %s: %s",
-                              ifa->ifa_name, strerror(errno));
-                }
-
-            } else {
-#ifdef CONFIG_SOLARIS
-                mac_addr = (unsigned char *) &ifr.ifr_addr.sa_data;
-#else
-                mac_addr = (unsigned char *) &ifr.ifr_hwaddr.sa_data;
-#endif
+            if (ret == 1) {
                 info->hardware_address =
                     g_strdup_printf("%02x:%02x:%02x:%02x:%02x:%02x",
                                     (int) mac_addr[0], (int) mac_addr[1],
                                     (int) mac_addr[2], (int) mac_addr[3],
                                     (int) mac_addr[4], (int) mac_addr[5]);
-
                 info->has_hardware_address = true;
             }
-            close(sock);
         }
 
         if (ifa->ifa_addr &&
-- 
2.34.1



^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH 7/7] qga: Add HW address getting for FreeBSD
  2022-09-29  7:52 [PATCH v2 0/7] qga: Add FreeBSD support Alexander Ivanov
                   ` (5 preceding siblings ...)
  2022-09-29  7:52 ` [PATCH 6/7] qga: Move HW address getting to a separate function Alexander Ivanov
@ 2022-09-29  7:52 ` Alexander Ivanov
  2022-09-29 11:28   ` Marc-André Lureau
  6 siblings, 1 reply; 20+ messages in thread
From: Alexander Ivanov @ 2022-09-29  7:52 UTC (permalink / raw)
  To: qemu-devel; +Cc: den, michael.roth, kkostiuk

Replace a dumb function in commands-bsd.c by the code of HW address
getting.

Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
---
 qga/commands-bsd.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/qga/commands-bsd.c b/qga/commands-bsd.c
index ca81114171..f9997ee1ec 100644
--- a/qga/commands-bsd.c
+++ b/qga/commands-bsd.c
@@ -20,6 +20,8 @@
 #include <sys/param.h>
 #include <sys/ucred.h>
 #include <sys/mount.h>
+#include <net/if_dl.h>
+#include <net/ethernet.h>
 #include <paths.h>
 
 #if defined(CONFIG_FSFREEZE) || defined(CONFIG_FSTRIM)
@@ -178,10 +180,15 @@ GuestCpuStatsList *qmp_guest_get_cpustats(Error **errp)
  */
 int guest_get_hw_addr(struct ifaddrs *ifa, unsigned char *buf, Error **errp)
 {
-    /*
-     * We can't get the hw addr of this interface,
-     * but that's not a fatal error.
-     */
-    return 0;
+    struct sockaddr_dl *sdp;
+
+    if (ifa->ifa_addr->sa_family != AF_LINK) {
+        /* We can get HW address only for AF_LINK family */
+        g_debug("failed to get MAC address of %s", ifa->ifa_name);
+        return 0;
+    }
+    sdp = (struct sockaddr_dl *)ifa->ifa_addr;
+    memcpy(buf, sdp->sdl_data + sdp->sdl_nlen, ETHER_ADDR_LEN);
+    return 1;
 }
 #endif /* HAVE_GETIFADDRS */
-- 
2.34.1



^ permalink raw reply related	[flat|nested] 20+ messages in thread

* Re: [PATCH 5/7] qga: Add support for user password setting in FreeBSD
  2022-09-29  7:52 ` [PATCH 5/7] qga: Add support for user password setting in FreeBSD Alexander Ivanov
@ 2022-09-29 11:22   ` Marc-André Lureau
  2022-09-29 14:29     ` Alexander Ivanov
  0 siblings, 1 reply; 20+ messages in thread
From: Marc-André Lureau @ 2022-09-29 11:22 UTC (permalink / raw)
  To: Alexander Ivanov; +Cc: qemu-devel, den, michael.roth, kkostiuk

[-- Attachment #1: Type: text/plain, Size: 8135 bytes --]

Hi Alexander

On Thu, Sep 29, 2022 at 12:52 PM Alexander Ivanov <
alexander.ivanov@virtuozzo.com> wrote:

> Move qmp_guest_set_user_password() from __linux__ condition to
> (__linux__ || __FreeBSD__) condition. Add command and arguments
> for password setting in FreeBSD.
>
> Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
>

Could you explain why you need to move the code?

You could instead have a top declaration?

If it's really required, please split the patch in move + additions.


> ---
>  qga/commands-posix.c | 223 +++++++++++++++++++++++--------------------
>  1 file changed, 118 insertions(+), 105 deletions(-)
>
> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> index 88e0d0fe24..6ce894ca6e 100644
> --- a/qga/commands-posix.c
> +++ b/qga/commands-posix.c
> @@ -2123,111 +2123,6 @@ int64_t
> qmp_guest_set_vcpus(GuestLogicalProcessorList *vcpus, Error **errp)
>      return processed;
>  }
>
> -void qmp_guest_set_user_password(const char *username,
> -                                 const char *password,
> -                                 bool crypted,
> -                                 Error **errp)
> -{
> -    Error *local_err = NULL;
> -    char *passwd_path = NULL;
> -    pid_t pid;
> -    int status;
> -    int datafd[2] = { -1, -1 };
> -    char *rawpasswddata = NULL;
> -    size_t rawpasswdlen;
> -    char *chpasswddata = NULL;
> -    size_t chpasswdlen;
> -
> -    rawpasswddata = (char *)qbase64_decode(password, -1, &rawpasswdlen,
> errp);
> -    if (!rawpasswddata) {
> -        return;
> -    }
> -    rawpasswddata = g_renew(char, rawpasswddata, rawpasswdlen + 1);
> -    rawpasswddata[rawpasswdlen] = '\0';
> -
> -    if (strchr(rawpasswddata, '\n')) {
> -        error_setg(errp, "forbidden characters in raw password");
> -        goto out;
> -    }
> -
> -    if (strchr(username, '\n') ||
> -        strchr(username, ':')) {
> -        error_setg(errp, "forbidden characters in username");
> -        goto out;
> -    }
> -
> -    chpasswddata = g_strdup_printf("%s:%s\n", username, rawpasswddata);
> -    chpasswdlen = strlen(chpasswddata);
> -
> -    passwd_path = g_find_program_in_path("chpasswd");
> -
> -    if (!passwd_path) {
> -        error_setg(errp, "cannot find 'passwd' program in PATH");
> -        goto out;
> -    }
> -
> -    if (!g_unix_open_pipe(datafd, FD_CLOEXEC, NULL)) {
> -        error_setg(errp, "cannot create pipe FDs");
> -        goto out;
> -    }
> -
> -    pid = fork();
> -    if (pid == 0) {
> -        close(datafd[1]);
> -        /* child */
> -        setsid();
> -        dup2(datafd[0], 0);
> -        reopen_fd_to_null(1);
> -        reopen_fd_to_null(2);
> -
> -        if (crypted) {
> -            execl(passwd_path, "chpasswd", "-e", NULL);
> -        } else {
> -            execl(passwd_path, "chpasswd", NULL);
> -        }
> -        _exit(EXIT_FAILURE);
> -    } else if (pid < 0) {
> -        error_setg_errno(errp, errno, "failed to create child process");
> -        goto out;
> -    }
> -    close(datafd[0]);
> -    datafd[0] = -1;
> -
> -    if (qemu_write_full(datafd[1], chpasswddata, chpasswdlen) !=
> chpasswdlen) {
> -        error_setg_errno(errp, errno, "cannot write new account
> password");
> -        goto out;
> -    }
> -    close(datafd[1]);
> -    datafd[1] = -1;
> -
> -    ga_wait_child(pid, &status, &local_err);
> -    if (local_err) {
> -        error_propagate(errp, local_err);
> -        goto out;
> -    }
> -
> -    if (!WIFEXITED(status)) {
> -        error_setg(errp, "child process has terminated abnormally");
> -        goto out;
> -    }
> -
> -    if (WEXITSTATUS(status)) {
> -        error_setg(errp, "child process has failed to set user password");
> -        goto out;
> -    }
> -
> -out:
> -    g_free(chpasswddata);
> -    g_free(rawpasswddata);
> -    g_free(passwd_path);
> -    if (datafd[0] != -1) {
> -        close(datafd[0]);
> -    }
> -    if (datafd[1] != -1) {
> -        close(datafd[1]);
> -    }
> -}
> -
>  static void ga_read_sysfs_file(int dirfd, const char *pathname, char *buf,
>                                 int size, Error **errp)
>  {
> @@ -2793,6 +2688,124 @@ GuestMemoryBlockInfo
> *qmp_guest_get_memory_block_info(Error **errp)
>
>  #endif
>
> +#if defined(__linux__) || defined(__FreeBSD__)
> +void qmp_guest_set_user_password(const char *username,
> +                                 const char *password,
> +                                 bool crypted,
> +                                 Error **errp)
> +{
> +    Error *local_err = NULL;
> +    char *passwd_path = NULL;
> +    pid_t pid;
> +    int status;
> +    int datafd[2] = { -1, -1 };
> +    char *rawpasswddata = NULL;
> +    size_t rawpasswdlen;
> +    char *chpasswddata = NULL;
> +    size_t chpasswdlen;
> +
> +    rawpasswddata = (char *)qbase64_decode(password, -1, &rawpasswdlen,
> errp);
> +    if (!rawpasswddata) {
> +        return;
> +    }
> +    rawpasswddata = g_renew(char, rawpasswddata, rawpasswdlen + 1);
> +    rawpasswddata[rawpasswdlen] = '\0';
> +
> +    if (strchr(rawpasswddata, '\n')) {
> +        error_setg(errp, "forbidden characters in raw password");
> +        goto out;
> +    }
> +
> +    if (strchr(username, '\n') ||
> +        strchr(username, ':')) {
> +        error_setg(errp, "forbidden characters in username");
> +        goto out;
> +    }
> +
> +#ifdef __FreeBSD__
> +    chpasswddata = g_strdup(rawpasswddata);
> +    passwd_path = g_find_program_in_path("pw");
> +#else
> +    chpasswddata = g_strdup_printf("%s:%s\n", username, rawpasswddata);
> +    passwd_path = g_find_program_in_path("chpasswd");
> +#endif
> +
> +    chpasswdlen = strlen(chpasswddata);
> +
> +    if (!passwd_path) {
> +        error_setg(errp, "cannot find 'passwd' program in PATH");
> +        goto out;
> +    }
> +
> +    if (!g_unix_open_pipe(datafd, FD_CLOEXEC, NULL)) {
> +        error_setg(errp, "cannot create pipe FDs");
> +        goto out;
> +    }
> +
> +    pid = fork();
> +    if (pid == 0) {
> +        close(datafd[1]);
> +        /* child */
> +        setsid();
> +        dup2(datafd[0], 0);
> +        reopen_fd_to_null(1);
> +        reopen_fd_to_null(2);
> +
> +#ifdef __FreeBSD__
> +        const char *h_arg;
> +        h_arg = (crypted) ? "-H" : "-h";
> +        execl(passwd_path, "pw", "usermod", "-n", username, h_arg, "0",
> NULL);
> +#else
> +        if (crypted) {
> +            execl(passwd_path, "chpasswd", "-e", NULL);
> +        } else {
> +            execl(passwd_path, "chpasswd", NULL);
> +        }
> +#endif
> +        _exit(EXIT_FAILURE);
> +    } else if (pid < 0) {
> +        error_setg_errno(errp, errno, "failed to create child process");
> +        goto out;
> +    }
> +    close(datafd[0]);
> +    datafd[0] = -1;
> +
> +    if (qemu_write_full(datafd[1], chpasswddata, chpasswdlen) !=
> chpasswdlen) {
> +        error_setg_errno(errp, errno, "cannot write new account
> password");
> +        goto out;
> +    }
> +    close(datafd[1]);
> +    datafd[1] = -1;
> +
> +    ga_wait_child(pid, &status, &local_err);
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        goto out;
> +    }
> +
> +    if (!WIFEXITED(status)) {
> +        error_setg(errp, "child process has terminated abnormally");
> +        goto out;
> +    }
> +
> +    if (WEXITSTATUS(status)) {
> +        error_setg(errp, "child process has failed to set user password");
> +        goto out;
> +    }
> +
> +out:
> +    g_free(chpasswddata);
> +    g_free(rawpasswddata);
> +    g_free(passwd_path);
> +    if (datafd[0] != -1) {
> +        close(datafd[0]);
> +    }
> +    if (datafd[1] != -1) {
> +        close(datafd[1]);
> +    }
> +}
> +#endif
> +
>  #ifdef HAVE_GETIFADDRS
>  static GuestNetworkInterface *
>  guest_find_interface(GuestNetworkInterfaceList *head,
> --
> 2.34.1
>
>
>

-- 
Marc-André Lureau

[-- Attachment #2: Type: text/html, Size: 10351 bytes --]

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 6/7] qga: Move HW address getting to a separate function
  2022-09-29  7:52 ` [PATCH 6/7] qga: Move HW address getting to a separate function Alexander Ivanov
@ 2022-09-29 11:28   ` Marc-André Lureau
  2022-09-29 14:33     ` Alexander Ivanov
  0 siblings, 1 reply; 20+ messages in thread
From: Marc-André Lureau @ 2022-09-29 11:28 UTC (permalink / raw)
  To: Alexander Ivanov; +Cc: qemu-devel, den, michael.roth, kkostiuk

[-- Attachment #1: Type: text/plain, Size: 8638 bytes --]

Hi

On Thu, Sep 29, 2022 at 12:02 PM Alexander Ivanov <
alexander.ivanov@virtuozzo.com> wrote:

> In the next patch FreeBSD support for guest-network-get-interfaces will be
> added. Previously move Linux-specific code of HW address getting to a
> separate functions and add a dumb function to commands-bsd.c.
>
> Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
> ---
>  qga/commands-bsd.c    |  18 +++++++
>  qga/commands-common.h |   6 +++
>  qga/commands-posix.c  | 114 +++++++++++++++++++++++-------------------
>  3 files changed, 87 insertions(+), 51 deletions(-)
>
> diff --git a/qga/commands-bsd.c b/qga/commands-bsd.c
> index ca06692179..ca81114171 100644
> --- a/qga/commands-bsd.c
> +++ b/qga/commands-bsd.c
> @@ -167,3 +167,21 @@ GuestCpuStatsList *qmp_guest_get_cpustats(Error
> **errp)
>      return NULL;
>  }
>  #endif /* CONFIG_FSFREEZE */
> +
> +#ifdef HAVE_GETIFADDRS
> +/*
> + * Fill buf with MAC address by ifaddrs. Pointer buf must point to a
> + * buffer with ETHER_ADDR_LEN length at least.
> + *
> + * Returns -1 in case of an error, 0 if MAC address can't be obtained or
> + * 1 if MAC addres is obtained.
>

Not a typical Error function return value...

Eventually, you could return a bool for error/ok and take an additional
"bool *obtained/valid" argument. Just a suggestion.



> + */
> +int guest_get_hw_addr(struct ifaddrs *ifa, unsigned char *buf, Error
> **errp)
> +{
> +    /*
> +     * We can't get the hw addr of this interface,
> +     * but that's not a fatal error.
> +     */
> +    return 0;
> +}
> +#endif /* HAVE_GETIFADDRS */
> diff --git a/qga/commands-common.h b/qga/commands-common.h
> index 2d9878a634..2485a037fd 100644
> --- a/qga/commands-common.h
> +++ b/qga/commands-common.h
> @@ -56,6 +56,12 @@ int64_t qmp_guest_fsfreeze_do_freeze_list(bool
> has_mountpoints,
>  int qmp_guest_fsfreeze_do_thaw(Error **errp);
>  #endif /* CONFIG_FSFREEZE */
>
> +#ifdef HAVE_GETIFADDRS
> +#include <ifaddrs.h>
> +
> +int guest_get_hw_addr(struct ifaddrs *ifa, unsigned char *buf, Error
> **errp);
> +#endif
> +
>  typedef struct GuestFileHandle GuestFileHandle;
>
>  GuestFileHandle *guest_file_handle_find(int64_t id, Error **errp);
> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> index 6ce894ca6e..0bebd9e690 100644
> --- a/qga/commands-posix.c
> +++ b/qga/commands-posix.c
> @@ -41,20 +41,12 @@
>  #endif
>  #endif
>
> -#ifdef __FreeBSD__
> -/*
> - * The code under HAVE_GETIFADDRS condition can't be compiled in FreeBSD.
> - * Fix it in one of the following patches.
> - */
> -#undef HAVE_GETIFADDRS
> -#endif
> -
>  #ifdef HAVE_GETIFADDRS
>  #include <arpa/inet.h>
>  #include <sys/socket.h>
>  #include <net/if.h>
> +#include <net/ethernet.h>
>  #include <sys/types.h>
> -#include <ifaddrs.h>
>  #ifdef CONFIG_SOLARIS
>  #include <sys/sockio.h>
>  #endif
> @@ -2659,14 +2651,6 @@ int64_t
> qmp_guest_set_vcpus(GuestLogicalProcessorList *vcpus, Error **errp)
>      return -1;
>  }
>
> -void qmp_guest_set_user_password(const char *username,
> -                                 const char *password,
> -                                 bool crypted,
> -                                 Error **errp)
> -{
> -    error_setg(errp, QERR_UNSUPPORTED);
> -}
> -
>

Why this in this patch?


>  GuestMemoryBlockList *qmp_guest_get_memory_blocks(Error **errp)
>  {
>      error_setg(errp, QERR_UNSUPPORTED);
> @@ -2804,7 +2788,15 @@ out:
>          close(datafd[1]);
>      }
>  }
> -#endif
> +#else /* __linux__ || __FreeBSD__ */
> +void qmp_guest_set_user_password(const char *username,
> +                                 const char *password,
> +                                 bool crypted,
> +                                 Error **errp)
> +{
> +    error_setg(errp, QERR_UNSUPPORTED);
> +}
> +#endif /* __linux__ || __FreeBSD__ */
>
>  #ifdef HAVE_GETIFADDRS
>  static GuestNetworkInterface *
> @@ -2887,6 +2879,54 @@ static int guest_get_network_stats(const char *name,
>      return -1;
>  }
>
> +#ifndef __FreeBSD__
> +/*
> + * Fill buf with MAC address by ifaddrs. Pointer buf must point to a
> + * buffer with ETHER_ADDR_LEN length at least.
> + * Returns -1 in case of an error, 0 if MAC address can't be obtained or
> + * 1 if MAC addres is obtained.
> + */
> +int guest_get_hw_addr(struct ifaddrs *ifa, unsigned char *buf, Error
> **errp)
> +{
> +    struct ifreq ifr;
> +    int sock;
> +
> +    /* we haven't obtained HW address yet */
> +    sock = socket(PF_INET, SOCK_STREAM, 0);
> +    if (sock == -1) {
> +        error_setg_errno(errp, errno, "failed to create socket");
> +        return -1;
> +    }
> +
> +    memset(&ifr, 0, sizeof(ifr));
> +    pstrcpy(ifr.ifr_name, IF_NAMESIZE, ifa->ifa_name);
> +    if (ioctl(sock, SIOCGIFHWADDR, &ifr) == -1) {
> +        /*
> +         * We can't get the hw addr of this interface, but that's not a
> +         * fatal error. Don't set info->hardware_address, but keep
> +         * going.
> +         */
> +        if (errno == EADDRNOTAVAIL) {
> +            /* The interface doesn't have a hw addr (e.g. loopback). */
> +            g_debug("failed to get MAC address of %s: %s",
> +                    ifa->ifa_name, strerror(errno));
> +        } else{
> +            g_warning("failed to get MAC address of %s: %s",
> +                      ifa->ifa_name, strerror(errno));
> +        }
> +        close(sock);
> +        return 0;
> +    }
> +#ifdef CONFIG_SOLARIS
> +    memcpy(buf, &ifr.ifr_addr.sa_data, ETHER_ADDR_LEN);
> +#else
> +    memcpy(buf, &ifr.ifr_hwaddr.sa_data, ETHER_ADDR_LEN);
> +#endif
> +    close(sock);
> +    return 1;
> +}
> +#endif /* __FreeBSD__ */
> +
>  /*
>   * Build information about guest interfaces
>   */
> @@ -2907,10 +2947,9 @@ GuestNetworkInterfaceList
> *qmp_guest_network_get_interfaces(Error **errp)
>          GuestNetworkInterfaceStat *interface_stat = NULL;
>          char addr4[INET_ADDRSTRLEN];
>          char addr6[INET6_ADDRSTRLEN];
> -        int sock;
> -        struct ifreq ifr;
> -        unsigned char *mac_addr;
> +        unsigned char mac_addr[ETHER_ADDR_LEN];
>          void *p;
> +        int ret;
>
>          g_debug("Processing %s interface", ifa->ifa_name);
>
> @@ -2924,45 +2963,18 @@ GuestNetworkInterfaceList
> *qmp_guest_network_get_interfaces(Error **errp)
>          }
>
>          if (!info->has_hardware_address) {
> -            /* we haven't obtained HW address yet */
> -            sock = socket(PF_INET, SOCK_STREAM, 0);
> -            if (sock == -1) {
> -                error_setg_errno(errp, errno, "failed to create socket");
> +            ret = guest_get_hw_addr(ifa, mac_addr, errp);
> +            if (ret == -1) {
>                  goto error;
>              }
> -
> -            memset(&ifr, 0, sizeof(ifr));
> -            pstrcpy(ifr.ifr_name, IF_NAMESIZE, info->name);
> -            if (ioctl(sock, SIOCGIFHWADDR, &ifr) == -1) {
> -                /*
> -                 * We can't get the hw addr of this interface, but that's
> not a
> -                 * fatal error. Don't set info->hardware_address, but keep
> -                 * going.
> -                 */
> -                if (errno == EADDRNOTAVAIL) {
> -                    /* The interface doesn't have a hw addr (e.g.
> loopback). */
> -                    g_debug("failed to get MAC address of %s: %s",
> -                            ifa->ifa_name, strerror(errno));
> -                } else{
> -                    g_warning("failed to get MAC address of %s: %s",
> -                              ifa->ifa_name, strerror(errno));
> -                }
> -
> -            } else {
> -#ifdef CONFIG_SOLARIS
> -                mac_addr = (unsigned char *) &ifr.ifr_addr.sa_data;
> -#else
> -                mac_addr = (unsigned char *) &ifr.ifr_hwaddr.sa_data;
> -#endif
> +            if (ret == 1) {
>                  info->hardware_address =
>                      g_strdup_printf("%02x:%02x:%02x:%02x:%02x:%02x",
>                                      (int) mac_addr[0], (int) mac_addr[1],
>                                      (int) mac_addr[2], (int) mac_addr[3],
>                                      (int) mac_addr[4], (int) mac_addr[5]);
> -
>                  info->has_hardware_address = true;
>              }
> -            close(sock);
>          }
>
>          if (ifa->ifa_addr &&
> --
> 2.34.1
>
>
>
looks ok to me otherwise

-- 
Marc-André Lureau

[-- Attachment #2: Type: text/html, Size: 10881 bytes --]

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 7/7] qga: Add HW address getting for FreeBSD
  2022-09-29  7:52 ` [PATCH 7/7] qga: Add HW address getting for FreeBSD Alexander Ivanov
@ 2022-09-29 11:28   ` Marc-André Lureau
  0 siblings, 0 replies; 20+ messages in thread
From: Marc-André Lureau @ 2022-09-29 11:28 UTC (permalink / raw)
  To: Alexander Ivanov; +Cc: qemu-devel, den, michael.roth, kkostiuk

[-- Attachment #1: Type: text/plain, Size: 1642 bytes --]

Hi

On Thu, Sep 29, 2022 at 11:54 AM Alexander Ivanov <
alexander.ivanov@virtuozzo.com> wrote:

> Replace a dumb function in commands-bsd.c by the code of HW address
> getting.
>
> Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
>

lgtm
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>



> ---
>  qga/commands-bsd.c | 17 ++++++++++++-----
>  1 file changed, 12 insertions(+), 5 deletions(-)
>
> diff --git a/qga/commands-bsd.c b/qga/commands-bsd.c
> index ca81114171..f9997ee1ec 100644
> --- a/qga/commands-bsd.c
> +++ b/qga/commands-bsd.c
> @@ -20,6 +20,8 @@
>  #include <sys/param.h>
>  #include <sys/ucred.h>
>  #include <sys/mount.h>
> +#include <net/if_dl.h>
> +#include <net/ethernet.h>
>  #include <paths.h>
>
>  #if defined(CONFIG_FSFREEZE) || defined(CONFIG_FSTRIM)
> @@ -178,10 +180,15 @@ GuestCpuStatsList *qmp_guest_get_cpustats(Error
> **errp)
>   */
>  int guest_get_hw_addr(struct ifaddrs *ifa, unsigned char *buf, Error
> **errp)
>  {
> -    /*
> -     * We can't get the hw addr of this interface,
> -     * but that's not a fatal error.
> -     */
> -    return 0;
> +    struct sockaddr_dl *sdp;
> +
> +    if (ifa->ifa_addr->sa_family != AF_LINK) {
> +        /* We can get HW address only for AF_LINK family */
> +        g_debug("failed to get MAC address of %s", ifa->ifa_name);
> +        return 0;
> +    }
> +    sdp = (struct sockaddr_dl *)ifa->ifa_addr;
> +    memcpy(buf, sdp->sdl_data + sdp->sdl_nlen, ETHER_ADDR_LEN);
> +    return 1;
>  }
>  #endif /* HAVE_GETIFADDRS */
> --
> 2.34.1
>
>
>

-- 
Marc-André Lureau

[-- Attachment #2: Type: text/html, Size: 2531 bytes --]

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 4/7] qga: Add shutdown/halt/reboot support for FreeBSD
  2022-09-29  7:52 ` [PATCH 4/7] qga: Add shutdown/halt/reboot " Alexander Ivanov
@ 2022-09-29 11:28   ` Marc-André Lureau
  0 siblings, 0 replies; 20+ messages in thread
From: Marc-André Lureau @ 2022-09-29 11:28 UTC (permalink / raw)
  To: Alexander Ivanov; +Cc: qemu-devel, den, michael.roth, kkostiuk

[-- Attachment #1: Type: text/plain, Size: 1662 bytes --]

On Thu, Sep 29, 2022 at 1:02 PM Alexander Ivanov <
alexander.ivanov@virtuozzo.com> wrote:

> Add appropriate shutdown command arguments to qmp_guest_shutdown()
> for FreeBSD.
>
> Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
>

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>


> ---
>  qga/commands-posix.c | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> index 49f9996a9c..88e0d0fe24 100644
> --- a/qga/commands-posix.c
> +++ b/qga/commands-posix.c
> @@ -90,6 +90,10 @@ void qmp_guest_shutdown(bool has_mode, const char
> *mode, Error **errp)
>      const char *powerdown_flag = "-i5";
>      const char *halt_flag = "-i0";
>      const char *reboot_flag = "-i6";
> +#elif defined(CONFIG_BSD)
> +    const char *powerdown_flag = "-p";
> +    const char *halt_flag = "-h";
> +    const char *reboot_flag = "-r";
>  #else
>      const char *powerdown_flag = "-P";
>      const char *halt_flag = "-H";
> @@ -120,6 +124,9 @@ void qmp_guest_shutdown(bool has_mode, const char
> *mode, Error **errp)
>  #ifdef CONFIG_SOLARIS
>          execl("/sbin/shutdown", "shutdown", shutdown_flag, "-g0", "-y",
>                "hypervisor initiated shutdown", (char *)NULL);
> +#elif defined(CONFIG_BSD)
> +        execl("/sbin/shutdown", "shutdown", shutdown_flag, "+0",
> +               "hypervisor initiated shutdown", (char *)NULL);
>  #else
>          execl("/sbin/shutdown", "shutdown", "-h", shutdown_flag, "+0",
>                 "hypervisor initiated shutdown", (char *)NULL);
> --
> 2.34.1
>
>
>

-- 
Marc-André Lureau

[-- Attachment #2: Type: text/html, Size: 2695 bytes --]

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 3/7] qga: Add UFS freeze/thaw support for FreeBSD
  2022-09-29  7:52 ` [PATCH 3/7] qga: Add UFS freeze/thaw support for FreeBSD Alexander Ivanov
@ 2022-09-29 11:28   ` Marc-André Lureau
  0 siblings, 0 replies; 20+ messages in thread
From: Marc-André Lureau @ 2022-09-29 11:28 UTC (permalink / raw)
  To: Alexander Ivanov; +Cc: qemu-devel, den, michael.roth, kkostiuk

[-- Attachment #1: Type: text/plain, Size: 17195 bytes --]

On Thu, Sep 29, 2022 at 12:04 PM Alexander Ivanov <
alexander.ivanov@virtuozzo.com> wrote:

> UFS supports FS freezing through ioctl UFSSUSPEND on /dev/ufssuspend.
> Frozen FS can be thawed by closing /dev/ufssuspend file descriptior.
>
> Use getmntinfo to get a list of mounted FS.
>
> Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
>

lgtm,
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>


> ---
>  qga/commands-bsd.c    | 169 +++++++++++++++++++++++
>  qga/commands-common.h |  11 ++
>  qga/commands-posix.c  | 308 ++++++++++++++++++++----------------------
>  qga/main.c            |   7 +-
>  qga/meson.build       |   3 +
>  5 files changed, 334 insertions(+), 164 deletions(-)
>  create mode 100644 qga/commands-bsd.c
>
> diff --git a/qga/commands-bsd.c b/qga/commands-bsd.c
> new file mode 100644
> index 0000000000..ca06692179
> --- /dev/null
> +++ b/qga/commands-bsd.c
> @@ -0,0 +1,169 @@
> +/*
> + * QEMU Guest Agent BSD-specific command implementations
> + *
> + * Copyright (c) Virtuozzo International GmbH.
> + *
> + * Authors:
> + *  Alexander Ivanov  <alexander.ivanov@virtuozzo.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 "qemu/osdep.h"
> +#include "qga-qapi-commands.h"
> +#include "qapi/qmp/qerror.h"
> +#include "qapi/error.h"
> +#include "qemu/queue.h"
> +#include "commands-common.h"
> +#include <sys/ioctl.h>
> +#include <sys/param.h>
> +#include <sys/ucred.h>
> +#include <sys/mount.h>
> +#include <paths.h>
> +
> +#if defined(CONFIG_FSFREEZE) || defined(CONFIG_FSTRIM)
> +bool build_fs_mount_list(FsMountList *mounts, Error **errp)
> +{
> +    FsMount *mount;
> +    struct statfs *mntbuf, *mntp;
> +    struct stat statbuf;
> +    int i, count, ret;
> +
> +    count = getmntinfo(&mntbuf, MNT_NOWAIT);
> +    if (count == 0) {
> +        error_setg_errno(errp, errno, "getmntinfo failed");
> +        return false;
> +    }
> +
> +    for (i = 0; i < count; i++) {
> +        mntp = &mntbuf[i];
> +        ret = stat(mntp->f_mntonname, &statbuf);
> +        if (ret != 0) {
> +            error_setg_errno(errp, errno, "stat failed on %s",
> +                             mntp->f_mntonname);
> +            return false;
> +        }
> +
> +        mount = g_new0(FsMount, 1);
> +
> +        mount->dirname = g_strdup(mntp->f_mntonname);
> +        mount->devtype = g_strdup(mntp->f_fstypename);
> +        mount->devmajor = major(mount->dev);
> +        mount->devminor = minor(mount->dev);
> +        mount->fsid = mntp->f_fsid;
> +        mount->dev = statbuf.st_dev;
> +
> +        QTAILQ_INSERT_TAIL(mounts, mount, next);
> +    }
> +    return true;
> +}
> +#endif /* CONFIG_FSFREEZE || CONFIG_FSTRIM */
> +
> +#if defined(CONFIG_FSFREEZE)
> +static int ufssuspend_fd = -1;
> +static int ufssuspend_cnt;
> +
> +int64_t qmp_guest_fsfreeze_do_freeze_list(bool has_mountpoints,
> +                                          strList *mountpoints,
> +                                          FsMountList mounts,
> +                                          Error **errp)
> +{
> +    int ret;
> +    strList *list;
> +    struct FsMount *mount;
> +
> +    if (ufssuspend_fd != -1) {
> +        error_setg(errp, "filesystems have already frozen");
> +        return -1;
> +    }
> +
> +    ufssuspend_cnt = 0;
> +    ufssuspend_fd = qemu_open(_PATH_UFSSUSPEND, O_RDWR, errp);
> +    if (ufssuspend_fd == -1) {
> +        return -1;
> +    }
> +
> +    QTAILQ_FOREACH_REVERSE(mount, &mounts, next) {
> +        /*
> +         * To issue fsfreeze in the reverse order of mounts, check if the
> +         * mount is listed in the list here
> +         */
> +        if (has_mountpoints) {
> +            for (list = mountpoints; list; list = list->next) {
> +                if (g_str_equal(list->value, mount->dirname)) {
> +                    break;
> +                }
> +            }
> +            if (!list) {
> +                continue;
> +            }
> +        }
> +
> +        /* Only UFS supports suspend */
> +        if (!g_str_equal(mount->devtype, "ufs")) {
> +            continue;
> +        }
> +
> +        ret = ioctl(ufssuspend_fd, UFSSUSPEND, &mount->fsid);
> +        if (ret == -1) {
> +            /*
> +             * ioctl returns EBUSY for all the FS except the first one
> +             * that was suspended
> +             */
> +            if (errno == EBUSY) {
> +                continue;
> +            }
> +            error_setg_errno(errp, errno, "failed to freeze %s",
> +                             mount->dirname);
> +            goto error;
> +        }
> +        ufssuspend_cnt++;
> +    }
> +    return ufssuspend_cnt;
> +error:
> +    close(ufssuspend_fd);
> +    ufssuspend_fd = -1;
> +    return -1;
> +
> +}
> +
> +/*
> + * We don't need to call UFSRESUME ioctl because all the frozen FS
> + * are thawed on /dev/ufssuspend closing.
> + */
> +int qmp_guest_fsfreeze_do_thaw(Error **errp)
> +{
> +    int ret = ufssuspend_cnt;
> +    ufssuspend_cnt = 0;
> +    if (ufssuspend_fd != -1) {
> +        close(ufssuspend_fd);
> +        ufssuspend_fd = -1;
> +    }
> +    return ret;
> +}
> +
> +GuestFilesystemInfoList *qmp_guest_get_fsinfo(Error **errp)
> +{
> +    error_setg(errp, QERR_UNSUPPORTED);
> +    return NULL;
> +}
> +
> +GuestDiskInfoList *qmp_guest_get_disks(Error **errp)
> +{
> +    error_setg(errp, QERR_UNSUPPORTED);
> +    return NULL;
> +}
> +
> +GuestDiskStatsInfoList *qmp_guest_get_diskstats(Error **errp)
> +{
> +    error_setg(errp, QERR_UNSUPPORTED);
> +    return NULL;
> +}
> +
> +GuestCpuStatsList *qmp_guest_get_cpustats(Error **errp)
> +{
> +    error_setg(errp, QERR_UNSUPPORTED);
> +    return NULL;
> +}
> +#endif /* CONFIG_FSFREEZE */
> diff --git a/qga/commands-common.h b/qga/commands-common.h
> index 181fc330aa..2d9878a634 100644
> --- a/qga/commands-common.h
> +++ b/qga/commands-common.h
> @@ -23,11 +23,22 @@
>  #endif
>  #endif /* __linux__ */
>
> +#ifdef __FreeBSD__
> +#include <ufs/ffs/fs.h>
> +#ifdef UFSSUSPEND
> +#define CONFIG_FSFREEZE
> +#endif
> +#endif /* __FreeBSD__ */
> +
>  #if defined(CONFIG_FSFREEZE) || defined(CONFIG_FSTRIM)
>  typedef struct FsMount {
>      char *dirname;
>      char *devtype;
>      unsigned int devmajor, devminor;
> +#if defined(__FreeBSD__)
> +    dev_t dev;
> +    fsid_t fsid;
> +#endif
>      QTAILQ_ENTRY(FsMount) next;
>  } FsMount;
>
> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> index 9574b83c92..49f9996a9c 100644
> --- a/qga/commands-posix.c
> +++ b/qga/commands-posix.c
> @@ -33,20 +33,12 @@
>
>  #if defined(__linux__)
>  #include <mntent.h>
> -#include <linux/fs.h>
>  #include <sys/statvfs.h>
>  #include <linux/nvme_ioctl.h>
>
>  #ifdef CONFIG_LIBUDEV
>  #include <libudev.h>
>  #endif
> -
> -#ifdef FIFREEZE
> -#define CONFIG_FSFREEZE
> -#endif
> -#ifdef FITRIM
> -#define CONFIG_FSTRIM
> -#endif
>  #endif
>
>  #ifdef __FreeBSD__
> @@ -623,9 +615,6 @@ void qmp_guest_file_flush(int64_t handle, Error **errp)
>      }
>  }
>
> -/* linux-specific implementations. avoid this if at all possible. */
> -#if defined(__linux__)
> -
>  #if defined(CONFIG_FSFREEZE) || defined(CONFIG_FSTRIM)
>  void free_fs_mount_list(FsMountList *mounts)
>  {
> @@ -644,6 +633,156 @@ void free_fs_mount_list(FsMountList *mounts)
>  }
>  #endif
>
> +#if defined(CONFIG_FSFREEZE)
> +typedef enum {
> +    FSFREEZE_HOOK_THAW = 0,
> +    FSFREEZE_HOOK_FREEZE,
> +} FsfreezeHookArg;
> +
> +static const char *fsfreeze_hook_arg_string[] = {
> +    "thaw",
> +    "freeze",
> +};
> +
> +static void execute_fsfreeze_hook(FsfreezeHookArg arg, Error **errp)
> +{
> +    int status;
> +    pid_t pid;
> +    const char *hook;
> +    const char *arg_str = fsfreeze_hook_arg_string[arg];
> +    Error *local_err = NULL;
> +
> +    hook = ga_fsfreeze_hook(ga_state);
> +    if (!hook) {
> +        return;
> +    }
> +    if (access(hook, X_OK) != 0) {
> +        error_setg_errno(errp, errno, "can't access fsfreeze hook '%s'",
> hook);
> +        return;
> +    }
> +
> +    slog("executing fsfreeze hook with arg '%s'", arg_str);
> +    pid = fork();
> +    if (pid == 0) {
> +        setsid();
> +        reopen_fd_to_null(0);
> +        reopen_fd_to_null(1);
> +        reopen_fd_to_null(2);
> +
> +        execl(hook, hook, arg_str, NULL);
> +        _exit(EXIT_FAILURE);
> +    } else if (pid < 0) {
> +        error_setg_errno(errp, errno, "failed to create child process");
> +        return;
> +    }
> +
> +    ga_wait_child(pid, &status, &local_err);
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }
> +
> +    if (!WIFEXITED(status)) {
> +        error_setg(errp, "fsfreeze hook has terminated abnormally");
> +        return;
> +    }
> +
> +    status = WEXITSTATUS(status);
> +    if (status) {
> +        error_setg(errp, "fsfreeze hook has failed with status %d",
> status);
> +        return;
> +    }
> +}
> +
> +/*
> + * Return status of freeze/thaw
> + */
> +GuestFsfreezeStatus qmp_guest_fsfreeze_status(Error **errp)
> +{
> +    if (ga_is_frozen(ga_state)) {
> +        return GUEST_FSFREEZE_STATUS_FROZEN;
> +    }
> +
> +    return GUEST_FSFREEZE_STATUS_THAWED;
> +}
> +
> +int64_t qmp_guest_fsfreeze_freeze(Error **errp)
> +{
> +    return qmp_guest_fsfreeze_freeze_list(false, NULL, errp);
> +}
> +
> +int64_t qmp_guest_fsfreeze_freeze_list(bool has_mountpoints,
> +                                       strList *mountpoints,
> +                                       Error **errp)
> +{
> +    int ret;
> +    FsMountList mounts;
> +    Error *local_err = NULL;
> +
> +    slog("guest-fsfreeze called");
> +
> +    execute_fsfreeze_hook(FSFREEZE_HOOK_FREEZE, &local_err);
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        return -1;
> +    }
> +
> +    QTAILQ_INIT(&mounts);
> +    if (!build_fs_mount_list(&mounts, &local_err)) {
> +        error_propagate(errp, local_err);
> +        return -1;
> +    }
> +
> +    /* cannot risk guest agent blocking itself on a write in this state */
> +    ga_set_frozen(ga_state);
> +
> +    ret = qmp_guest_fsfreeze_do_freeze_list(has_mountpoints, mountpoints,
> +                                            mounts, errp);
> +
> +    free_fs_mount_list(&mounts);
> +    /* We may not issue any FIFREEZE here.
> +     * Just unset ga_state here and ready for the next call.
> +     */
> +    if (ret == 0) {
> +        ga_unset_frozen(ga_state);
> +    } else if (ret < 0) {
> +        qmp_guest_fsfreeze_thaw(NULL);
> +    }
> +    return ret;
> +}
> +
> +int64_t qmp_guest_fsfreeze_thaw(Error **errp)
> +{
> +    int ret;
> +
> +    ret = qmp_guest_fsfreeze_do_thaw(errp);
> +    if (ret >= 0) {
> +        ga_unset_frozen(ga_state);
> +        execute_fsfreeze_hook(FSFREEZE_HOOK_THAW, errp);
> +    } else {
> +        ret = 0;
> +    }
> +
> +    return ret;
> +}
> +
> +static void guest_fsfreeze_cleanup(void)
> +{
> +    Error *err = NULL;
> +
> +    if (ga_is_frozen(ga_state) == GUEST_FSFREEZE_STATUS_FROZEN) {
> +        qmp_guest_fsfreeze_thaw(&err);
> +        if (err) {
> +            slog("failed to clean up frozen filesystems: %s",
> +                 error_get_pretty(err));
> +            error_free(err);
> +        }
> +    }
> +}
> +#endif
> +
> +/* linux-specific implementations. avoid this if at all possible. */
> +#if defined(__linux__)
>  #if defined(CONFIG_FSFREEZE)
>
>  static char *get_pci_driver(char const *syspath, int pathlen, Error
> **errp)
> @@ -1467,153 +1606,6 @@ GuestFilesystemInfoList
> *qmp_guest_get_fsinfo(Error **errp)
>      free_fs_mount_list(&mounts);
>      return ret;
>  }
> -
> -
> -typedef enum {
> -    FSFREEZE_HOOK_THAW = 0,
> -    FSFREEZE_HOOK_FREEZE,
> -} FsfreezeHookArg;
> -
> -static const char *fsfreeze_hook_arg_string[] = {
> -    "thaw",
> -    "freeze",
> -};
> -
> -static void execute_fsfreeze_hook(FsfreezeHookArg arg, Error **errp)
> -{
> -    int status;
> -    pid_t pid;
> -    const char *hook;
> -    const char *arg_str = fsfreeze_hook_arg_string[arg];
> -    Error *local_err = NULL;
> -
> -    hook = ga_fsfreeze_hook(ga_state);
> -    if (!hook) {
> -        return;
> -    }
> -    if (access(hook, X_OK) != 0) {
> -        error_setg_errno(errp, errno, "can't access fsfreeze hook '%s'",
> hook);
> -        return;
> -    }
> -
> -    slog("executing fsfreeze hook with arg '%s'", arg_str);
> -    pid = fork();
> -    if (pid == 0) {
> -        setsid();
> -        reopen_fd_to_null(0);
> -        reopen_fd_to_null(1);
> -        reopen_fd_to_null(2);
> -
> -        execl(hook, hook, arg_str, NULL);
> -        _exit(EXIT_FAILURE);
> -    } else if (pid < 0) {
> -        error_setg_errno(errp, errno, "failed to create child process");
> -        return;
> -    }
> -
> -    ga_wait_child(pid, &status, &local_err);
> -    if (local_err) {
> -        error_propagate(errp, local_err);
> -        return;
> -    }
> -
> -    if (!WIFEXITED(status)) {
> -        error_setg(errp, "fsfreeze hook has terminated abnormally");
> -        return;
> -    }
> -
> -    status = WEXITSTATUS(status);
> -    if (status) {
> -        error_setg(errp, "fsfreeze hook has failed with status %d",
> status);
> -        return;
> -    }
> -}
> -
> -/*
> - * Return status of freeze/thaw
> - */
> -GuestFsfreezeStatus qmp_guest_fsfreeze_status(Error **errp)
> -{
> -    if (ga_is_frozen(ga_state)) {
> -        return GUEST_FSFREEZE_STATUS_FROZEN;
> -    }
> -
> -    return GUEST_FSFREEZE_STATUS_THAWED;
> -}
> -
> -int64_t qmp_guest_fsfreeze_freeze(Error **errp)
> -{
> -    return qmp_guest_fsfreeze_freeze_list(false, NULL, errp);
> -}
> -
> -int64_t qmp_guest_fsfreeze_freeze_list(bool has_mountpoints,
> -                                       strList *mountpoints,
> -                                       Error **errp)
> -{
> -    int ret;
> -    FsMountList mounts;
> -    Error *local_err = NULL;
> -
> -    slog("guest-fsfreeze called");
> -
> -    execute_fsfreeze_hook(FSFREEZE_HOOK_FREEZE, &local_err);
> -    if (local_err) {
> -        error_propagate(errp, local_err);
> -        return -1;
> -    }
> -
> -    QTAILQ_INIT(&mounts);
> -    if (!build_fs_mount_list(&mounts, &local_err)) {
> -        error_propagate(errp, local_err);
> -        return -1;
> -    }
> -
> -    /* cannot risk guest agent blocking itself on a write in this state */
> -    ga_set_frozen(ga_state);
> -
> -    ret = qmp_guest_fsfreeze_do_freeze_list(has_mountpoints, mountpoints,
> -                                            mounts, errp);
> -
> -    free_fs_mount_list(&mounts);
> -    /* We may not issue any FIFREEZE here.
> -     * Just unset ga_state here and ready for the next call.
> -     */
> -    if (ret == 0) {
> -        ga_unset_frozen(ga_state);
> -    } else if (ret < 0) {
> -        qmp_guest_fsfreeze_thaw(NULL);
> -    }
> -    return ret;
> -}
> -
> -int64_t qmp_guest_fsfreeze_thaw(Error **errp)
> -{
> -    int ret;
> -
> -    ret = qmp_guest_fsfreeze_do_thaw(errp);
> -    if (ret >= 0) {
> -        ga_unset_frozen(ga_state);
> -        execute_fsfreeze_hook(FSFREEZE_HOOK_THAW, errp);
> -    } else {
> -        ret = 0;
> -    }
> -
> -    return ret;
> -}
> -
> -static void guest_fsfreeze_cleanup(void)
> -{
> -    Error *err = NULL;
> -
> -    if (ga_is_frozen(ga_state) == GUEST_FSFREEZE_STATUS_FROZEN) {
> -        qmp_guest_fsfreeze_thaw(&err);
> -        if (err) {
> -            slog("failed to clean up frozen filesystems: %s",
> -                 error_get_pretty(err));
> -            error_free(err);
> -        }
> -    }
> -}
>  #endif /* CONFIG_FSFREEZE */
>
>  #if defined(CONFIG_FSTRIM)
> diff --git a/qga/main.c b/qga/main.c
> index 0d27c97d38..b3580508fa 100644
> --- a/qga/main.c
> +++ b/qga/main.c
> @@ -37,12 +37,7 @@
>  #include "qga/service-win32.h"
>  #include "qga/vss-win32.h"
>  #endif
> -#ifdef __linux__
> -#include <linux/fs.h>
> -#ifdef FIFREEZE
> -#define CONFIG_FSFREEZE
> -#endif
> -#endif
> +#include "commands-common.h"
>
>  #ifndef _WIN32
>  #ifdef __FreeBSD__
> diff --git a/qga/meson.build b/qga/meson.build
> index 409f49a000..456ba4c29f 100644
> --- a/qga/meson.build
> +++ b/qga/meson.build
> @@ -75,6 +75,9 @@ qga_ss.add(when: 'CONFIG_POSIX', if_true: files(
>  qga_ss.add(when: 'CONFIG_LINUX', if_true: files(
>    'commands-linux.c',
>  ))
> +qga_ss.add(when: 'CONFIG_BSD', if_true: files(
> +  'commands-bsd.c',
> +))
>  qga_ss.add(when: 'CONFIG_WIN32', if_true: files(
>    'channel-win32.c',
>    'commands-win32.c',
> --
> 2.34.1
>
>
>

-- 
Marc-André Lureau

[-- Attachment #2: Type: text/html, Size: 20994 bytes --]

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 2/7] qga: Move Linux-specific FS freeze/thaw code to a separate file
  2022-09-29  7:52 ` [PATCH 2/7] qga: Move Linux-specific FS freeze/thaw code to a separate file Alexander Ivanov
@ 2022-09-29 11:29   ` Marc-André Lureau
  0 siblings, 0 replies; 20+ messages in thread
From: Marc-André Lureau @ 2022-09-29 11:29 UTC (permalink / raw)
  To: Alexander Ivanov; +Cc: qemu-devel, den, michael.roth, kkostiuk

[-- Attachment #1: Type: text/plain, Size: 25300 bytes --]

On Thu, Sep 29, 2022 at 12:16 PM Alexander Ivanov <
alexander.ivanov@virtuozzo.com> wrote:

> In the next patches we are going to add FreeBSD support for QEMU Guest
> Agent. In the result, code in commands-posix.c will be too cumbersome.
>
> Move Linux-specific FS freeze/thaw code to a separate file commands-linux.c
> keeping common POSIX code in commands-posix.c.
>
> Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
>

lgtm,
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>



> ---
>  qga/commands-common.h |  35 +++++
>  qga/commands-linux.c  | 286 +++++++++++++++++++++++++++++++++++++++++
>  qga/commands-posix.c  | 289 +++---------------------------------------
>  qga/meson.build       |   3 +
>  4 files changed, 340 insertions(+), 273 deletions(-)
>  create mode 100644 qga/commands-linux.c
>
> diff --git a/qga/commands-common.h b/qga/commands-common.h
> index d0e4a9696f..181fc330aa 100644
> --- a/qga/commands-common.h
> +++ b/qga/commands-common.h
> @@ -10,6 +10,40 @@
>  #define QGA_COMMANDS_COMMON_H
>
>  #include "qga-qapi-types.h"
> +#include "guest-agent-core.h"
> +#include "qemu/queue.h"
> +
> +#if defined(__linux__)
> +#include <linux/fs.h>
> +#ifdef FIFREEZE
> +#define CONFIG_FSFREEZE
> +#endif
> +#ifdef FITRIM
> +#define CONFIG_FSTRIM
> +#endif
> +#endif /* __linux__ */
> +
> +#if defined(CONFIG_FSFREEZE) || defined(CONFIG_FSTRIM)
> +typedef struct FsMount {
> +    char *dirname;
> +    char *devtype;
> +    unsigned int devmajor, devminor;
> +    QTAILQ_ENTRY(FsMount) next;
> +} FsMount;
> +
> +typedef QTAILQ_HEAD(FsMountList, FsMount) FsMountList;
> +
> +bool build_fs_mount_list(FsMountList *mounts, Error **errp);
> +void free_fs_mount_list(FsMountList *mounts);
> +#endif /* CONFIG_FSFREEZE || CONFIG_FSTRIM */
> +
> +#if defined(CONFIG_FSFREEZE)
> +int64_t qmp_guest_fsfreeze_do_freeze_list(bool has_mountpoints,
> +                                          strList *mountpoints,
> +                                          FsMountList mounts,
> +                                          Error **errp);
> +int qmp_guest_fsfreeze_do_thaw(Error **errp);
> +#endif /* CONFIG_FSFREEZE */
>
>  typedef struct GuestFileHandle GuestFileHandle;
>
> @@ -29,4 +63,5 @@ GuestFileRead *guest_file_read_unsafe(GuestFileHandle
> *gfh,
>   */
>  char *qga_get_host_name(Error **errp);
>
> +void ga_wait_child(pid_t pid, int *status, Error **errp);
>  #endif
> diff --git a/qga/commands-linux.c b/qga/commands-linux.c
> new file mode 100644
> index 0000000000..214e408fcd
> --- /dev/null
> +++ b/qga/commands-linux.c
> @@ -0,0 +1,286 @@
> +/*
> + * QEMU Guest Agent Linux-specific command implementations
> + *
> + * Copyright IBM Corp. 2011
> + *
> + * Authors:
> + *  Michael Roth      <mdroth@linux.vnet.ibm.com>
> + *  Michal Privoznik  <mprivozn@redhat.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 "qemu/osdep.h"
> +#include "qapi/error.h"
> +#include "commands-common.h"
> +#include "cutils.h"
> +#include <mntent.h>
> +#include <sys/ioctl.h>
> +
> +#if defined(CONFIG_FSFREEZE) || defined(CONFIG_FSTRIM)
> +static int dev_major_minor(const char *devpath,
> +                           unsigned int *devmajor, unsigned int *devminor)
> +{
> +    struct stat st;
> +
> +    *devmajor = 0;
> +    *devminor = 0;
> +
> +    if (stat(devpath, &st) < 0) {
> +        slog("failed to stat device file '%s': %s", devpath,
> strerror(errno));
> +        return -1;
> +    }
> +    if (S_ISDIR(st.st_mode)) {
> +        /* It is bind mount */
> +        return -2;
> +    }
> +    if (S_ISBLK(st.st_mode)) {
> +        *devmajor = major(st.st_rdev);
> +        *devminor = minor(st.st_rdev);
> +        return 0;
> +    }
> +    return -1;
> +}
> +
> +static bool build_fs_mount_list_from_mtab(FsMountList *mounts, Error
> **errp)
> +{
> +    struct mntent *ment;
> +    FsMount *mount;
> +    char const *mtab = "/proc/self/mounts";
> +    FILE *fp;
> +    unsigned int devmajor, devminor;
> +
> +    fp = setmntent(mtab, "r");
> +    if (!fp) {
> +        error_setg(errp, "failed to open mtab file: '%s'", mtab);
> +        return false;
> +    }
> +
> +    while ((ment = getmntent(fp))) {
> +        /*
> +         * An entry which device name doesn't start with a '/' is
> +         * either a dummy file system or a network file system.
> +         * Add special handling for smbfs and cifs as is done by
> +         * coreutils as well.
> +         */
> +        if ((ment->mnt_fsname[0] != '/') ||
> +            (strcmp(ment->mnt_type, "smbfs") == 0) ||
> +            (strcmp(ment->mnt_type, "cifs") == 0)) {
> +            continue;
> +        }
> +        if (dev_major_minor(ment->mnt_fsname, &devmajor, &devminor) ==
> -2) {
> +            /* Skip bind mounts */
> +            continue;
> +        }
> +
> +        mount = g_new0(FsMount, 1);
> +        mount->dirname = g_strdup(ment->mnt_dir);
> +        mount->devtype = g_strdup(ment->mnt_type);
> +        mount->devmajor = devmajor;
> +        mount->devminor = devminor;
> +
> +        QTAILQ_INSERT_TAIL(mounts, mount, next);
> +    }
> +
> +    endmntent(fp);
> +    return true;
> +}
> +
> +static void decode_mntname(char *name, int len)
> +{
> +    int i, j = 0;
> +    for (i = 0; i <= len; i++) {
> +        if (name[i] != '\\') {
> +            name[j++] = name[i];
> +        } else if (name[i + 1] == '\\') {
> +            name[j++] = '\\';
> +            i++;
> +        } else if (name[i + 1] >= '0' && name[i + 1] <= '3' &&
> +                   name[i + 2] >= '0' && name[i + 2] <= '7' &&
> +                   name[i + 3] >= '0' && name[i + 3] <= '7') {
> +            name[j++] = (name[i + 1] - '0') * 64 +
> +                        (name[i + 2] - '0') * 8 +
> +                        (name[i + 3] - '0');
> +            i += 3;
> +        } else {
> +            name[j++] = name[i];
> +        }
> +    }
> +}
> +
> +/*
> + * Walk the mount table and build a list of local file systems
> + */
> +bool build_fs_mount_list(FsMountList *mounts, Error **errp)
> +{
> +    FsMount *mount;
> +    char const *mountinfo = "/proc/self/mountinfo";
> +    FILE *fp;
> +    char *line = NULL, *dash;
> +    size_t n;
> +    char check;
> +    unsigned int devmajor, devminor;
> +    int ret, dir_s, dir_e, type_s, type_e, dev_s, dev_e;
> +
> +    fp = fopen(mountinfo, "r");
> +    if (!fp) {
> +        return build_fs_mount_list_from_mtab(mounts, errp);
> +    }
> +
> +    while (getline(&line, &n, fp) != -1) {
> +        ret = sscanf(line, "%*u %*u %u:%u %*s %n%*s%n%c",
> +                     &devmajor, &devminor, &dir_s, &dir_e, &check);
> +        if (ret < 3) {
> +            continue;
> +        }
> +        dash = strstr(line + dir_e, " - ");
> +        if (!dash) {
> +            continue;
> +        }
> +        ret = sscanf(dash, " - %n%*s%n %n%*s%n%c",
> +                     &type_s, &type_e, &dev_s, &dev_e, &check);
> +        if (ret < 1) {
> +            continue;
> +        }
> +        line[dir_e] = 0;
> +        dash[type_e] = 0;
> +        dash[dev_e] = 0;
> +        decode_mntname(line + dir_s, dir_e - dir_s);
> +        decode_mntname(dash + dev_s, dev_e - dev_s);
> +        if (devmajor == 0) {
> +            /* btrfs reports major number = 0 */
> +            if (strcmp("btrfs", dash + type_s) != 0 ||
> +                dev_major_minor(dash + dev_s, &devmajor, &devminor) < 0) {
> +                continue;
> +            }
> +        }
> +
> +        mount = g_new0(FsMount, 1);
> +        mount->dirname = g_strdup(line + dir_s);
> +        mount->devtype = g_strdup(dash + type_s);
> +        mount->devmajor = devmajor;
> +        mount->devminor = devminor;
> +
> +        QTAILQ_INSERT_TAIL(mounts, mount, next);
> +    }
> +    free(line);
> +
> +    fclose(fp);
> +    return true;
> +}
> +#endif /* CONFIG_FSFREEZE || CONFIG_FSTRIM */
> +
> +#ifdef CONFIG_FSFREEZE
> +/*
> + * Walk list of mounted file systems in the guest, and freeze the ones
> which
> + * are real local file systems.
> + */
> +int64_t qmp_guest_fsfreeze_do_freeze_list(bool has_mountpoints,
> +                                          strList *mountpoints,
> +                                          FsMountList mounts,
> +                                          Error **errp)
> +{
> +    struct FsMount *mount;
> +    strList *list;
> +    int fd, ret, i = 0;
> +
> +    QTAILQ_FOREACH_REVERSE(mount, &mounts, next) {
> +        /* To issue fsfreeze in the reverse order of mounts, check if the
> +         * mount is listed in the list here */
> +        if (has_mountpoints) {
> +            for (list = mountpoints; list; list = list->next) {
> +                if (strcmp(list->value, mount->dirname) == 0) {
> +                    break;
> +                }
> +            }
> +            if (!list) {
> +                continue;
> +            }
> +        }
> +
> +        fd = qga_open_cloexec(mount->dirname, O_RDONLY, 0);
> +        if (fd == -1) {
> +            error_setg_errno(errp, errno, "failed to open %s",
> mount->dirname);
> +            return -1;
> +        }
> +
> +        /* we try to cull filesystems we know won't work in advance, but
> other
> +         * filesystems may not implement fsfreeze for less obvious
> reasons.
> +         * these will report EOPNOTSUPP. we simply ignore these when
> tallying
> +         * the number of frozen filesystems.
> +         * if a filesystem is mounted more than once (aka bind mount) a
> +         * consecutive attempt to freeze an already frozen filesystem will
> +         * return EBUSY.
> +         *
> +         * any other error means a failure to freeze a filesystem we
> +         * expect to be freezable, so return an error in those cases
> +         * and return system to thawed state.
> +         */
> +        ret = ioctl(fd, FIFREEZE);
> +        if (ret == -1) {
> +            if (errno != EOPNOTSUPP && errno != EBUSY) {
> +                error_setg_errno(errp, errno, "failed to freeze %s",
> +                                 mount->dirname);
> +                close(fd);
> +                return -1;
> +            }
> +        } else {
> +            i++;
> +        }
> +        close(fd);
> +    }
> +    return i;
> +}
> +
> +int qmp_guest_fsfreeze_do_thaw(Error **errp)
> +{
> +    int ret;
> +    FsMountList mounts;
> +    FsMount *mount;
> +    int fd, i = 0, logged;
> +    Error *local_err = NULL;
> +
> +    QTAILQ_INIT(&mounts);
> +    if (!build_fs_mount_list(&mounts, &local_err)) {
> +        error_propagate(errp, local_err);
> +        return -1;
> +    }
> +
> +    QTAILQ_FOREACH(mount, &mounts, next) {
> +        logged = false;
> +        fd = qga_open_cloexec(mount->dirname, O_RDONLY, 0);
> +        if (fd == -1) {
> +            continue;
> +        }
> +        /* we have no way of knowing whether a filesystem was actually
> unfrozen
> +         * as a result of a successful call to FITHAW, only that if an
> error
> +         * was returned the filesystem was *not* unfrozen by that
> particular
> +         * call.
> +         *
> +         * since multiple preceding FIFREEZEs require multiple calls to
> FITHAW
> +         * to unfreeze, continuing issuing FITHAW until an error is
> returned,
> +         * in which case either the filesystem is in an unfreezable
> state, or,
> +         * more likely, it was thawed previously (and remains so
> afterward).
> +         *
> +         * also, since the most recent successful call is the one that did
> +         * the actual unfreeze, we can use this to provide an accurate
> count
> +         * of the number of filesystems unfrozen by guest-fsfreeze-thaw,
> which
> +         * may * be useful for determining whether a filesystem was
> unfrozen
> +         * during the freeze/thaw phase by a process other than qemu-ga.
> +         */
> +        do {
> +            ret = ioctl(fd, FITHAW);
> +            if (ret == 0 && !logged) {
> +                i++;
> +                logged = true;
> +            }
> +        } while (ret == 0);
> +        close(fd);
> +    }
> +
> +    free_fs_mount_list(&mounts);
> +
> +    return i;
> +}
> +#endif /* CONFIG_FSFREEZE */
> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> index 16d67e9f6d..9574b83c92 100644
> --- a/qga/commands-posix.c
> +++ b/qga/commands-posix.c
> @@ -16,11 +16,9 @@
>  #include <sys/utsname.h>
>  #include <sys/wait.h>
>  #include <dirent.h>
> -#include "guest-agent-core.h"
>  #include "qga-qapi-commands.h"
>  #include "qapi/error.h"
>  #include "qapi/qmp/qerror.h"
> -#include "qemu/queue.h"
>  #include "qemu/host-utils.h"
>  #include "qemu/sockets.h"
>  #include "qemu/base64.h"
> @@ -70,7 +68,7 @@
>  #endif
>  #endif
>
> -static void ga_wait_child(pid_t pid, int *status, Error **errp)
> +void ga_wait_child(pid_t pid, int *status, Error **errp)
>  {
>      pid_t rpid;
>
> @@ -629,16 +627,7 @@ void qmp_guest_file_flush(int64_t handle, Error
> **errp)
>  #if defined(__linux__)
>
>  #if defined(CONFIG_FSFREEZE) || defined(CONFIG_FSTRIM)
> -typedef struct FsMount {
> -    char *dirname;
> -    char *devtype;
> -    unsigned int devmajor, devminor;
> -    QTAILQ_ENTRY(FsMount) next;
> -} FsMount;
> -
> -typedef QTAILQ_HEAD(FsMountList, FsMount) FsMountList;
> -
> -static void free_fs_mount_list(FsMountList *mounts)
> +void free_fs_mount_list(FsMountList *mounts)
>  {
>       FsMount *mount, *temp;
>
> @@ -653,157 +642,6 @@ static void free_fs_mount_list(FsMountList *mounts)
>           g_free(mount);
>       }
>  }
> -
> -static int dev_major_minor(const char *devpath,
> -                           unsigned int *devmajor, unsigned int *devminor)
> -{
> -    struct stat st;
> -
> -    *devmajor = 0;
> -    *devminor = 0;
> -
> -    if (stat(devpath, &st) < 0) {
> -        slog("failed to stat device file '%s': %s", devpath,
> strerror(errno));
> -        return -1;
> -    }
> -    if (S_ISDIR(st.st_mode)) {
> -        /* It is bind mount */
> -        return -2;
> -    }
> -    if (S_ISBLK(st.st_mode)) {
> -        *devmajor = major(st.st_rdev);
> -        *devminor = minor(st.st_rdev);
> -        return 0;
> -    }
> -    return -1;
> -}
> -
> -/*
> - * Walk the mount table and build a list of local file systems
> - */
> -static bool build_fs_mount_list_from_mtab(FsMountList *mounts, Error
> **errp)
> -{
> -    struct mntent *ment;
> -    FsMount *mount;
> -    char const *mtab = "/proc/self/mounts";
> -    FILE *fp;
> -    unsigned int devmajor, devminor;
> -
> -    fp = setmntent(mtab, "r");
> -    if (!fp) {
> -        error_setg(errp, "failed to open mtab file: '%s'", mtab);
> -        return false;
> -    }
> -
> -    while ((ment = getmntent(fp))) {
> -        /*
> -         * An entry which device name doesn't start with a '/' is
> -         * either a dummy file system or a network file system.
> -         * Add special handling for smbfs and cifs as is done by
> -         * coreutils as well.
> -         */
> -        if ((ment->mnt_fsname[0] != '/') ||
> -            (strcmp(ment->mnt_type, "smbfs") == 0) ||
> -            (strcmp(ment->mnt_type, "cifs") == 0)) {
> -            continue;
> -        }
> -        if (dev_major_minor(ment->mnt_fsname, &devmajor, &devminor) ==
> -2) {
> -            /* Skip bind mounts */
> -            continue;
> -        }
> -
> -        mount = g_new0(FsMount, 1);
> -        mount->dirname = g_strdup(ment->mnt_dir);
> -        mount->devtype = g_strdup(ment->mnt_type);
> -        mount->devmajor = devmajor;
> -        mount->devminor = devminor;
> -
> -        QTAILQ_INSERT_TAIL(mounts, mount, next);
> -    }
> -
> -    endmntent(fp);
> -    return true;
> -}
> -
> -static void decode_mntname(char *name, int len)
> -{
> -    int i, j = 0;
> -    for (i = 0; i <= len; i++) {
> -        if (name[i] != '\\') {
> -            name[j++] = name[i];
> -        } else if (name[i + 1] == '\\') {
> -            name[j++] = '\\';
> -            i++;
> -        } else if (name[i + 1] >= '0' && name[i + 1] <= '3' &&
> -                   name[i + 2] >= '0' && name[i + 2] <= '7' &&
> -                   name[i + 3] >= '0' && name[i + 3] <= '7') {
> -            name[j++] = (name[i + 1] - '0') * 64 +
> -                        (name[i + 2] - '0') * 8 +
> -                        (name[i + 3] - '0');
> -            i += 3;
> -        } else {
> -            name[j++] = name[i];
> -        }
> -    }
> -}
> -
> -static bool build_fs_mount_list(FsMountList *mounts, Error **errp)
> -{
> -    FsMount *mount;
> -    char const *mountinfo = "/proc/self/mountinfo";
> -    FILE *fp;
> -    char *line = NULL, *dash;
> -    size_t n;
> -    char check;
> -    unsigned int devmajor, devminor;
> -    int ret, dir_s, dir_e, type_s, type_e, dev_s, dev_e;
> -
> -    fp = fopen(mountinfo, "r");
> -    if (!fp) {
> -        return build_fs_mount_list_from_mtab(mounts, errp);
> -    }
> -
> -    while (getline(&line, &n, fp) != -1) {
> -        ret = sscanf(line, "%*u %*u %u:%u %*s %n%*s%n%c",
> -                     &devmajor, &devminor, &dir_s, &dir_e, &check);
> -        if (ret < 3) {
> -            continue;
> -        }
> -        dash = strstr(line + dir_e, " - ");
> -        if (!dash) {
> -            continue;
> -        }
> -        ret = sscanf(dash, " - %n%*s%n %n%*s%n%c",
> -                     &type_s, &type_e, &dev_s, &dev_e, &check);
> -        if (ret < 1) {
> -            continue;
> -        }
> -        line[dir_e] = 0;
> -        dash[type_e] = 0;
> -        dash[dev_e] = 0;
> -        decode_mntname(line + dir_s, dir_e - dir_s);
> -        decode_mntname(dash + dev_s, dev_e - dev_s);
> -        if (devmajor == 0) {
> -            /* btrfs reports major number = 0 */
> -            if (strcmp("btrfs", dash + type_s) != 0 ||
> -                dev_major_minor(dash + dev_s, &devmajor, &devminor) < 0) {
> -                continue;
> -            }
> -        }
> -
> -        mount = g_new0(FsMount, 1);
> -        mount->dirname = g_strdup(line + dir_s);
> -        mount->devtype = g_strdup(dash + type_s);
> -        mount->devmajor = devmajor;
> -        mount->devminor = devminor;
> -
> -        QTAILQ_INSERT_TAIL(mounts, mount, next);
> -    }
> -    free(line);
> -
> -    fclose(fp);
> -    return true;
> -}
>  #endif
>
>  #if defined(CONFIG_FSFREEZE)
> @@ -1708,20 +1546,13 @@ int64_t qmp_guest_fsfreeze_freeze(Error **errp)
>      return qmp_guest_fsfreeze_freeze_list(false, NULL, errp);
>  }
>
> -/*
> - * Walk list of mounted file systems in the guest, and freeze the ones
> which
> - * are real local file systems.
> - */
>  int64_t qmp_guest_fsfreeze_freeze_list(bool has_mountpoints,
>                                         strList *mountpoints,
>                                         Error **errp)
>  {
> -    int ret = 0, i = 0;
> -    strList *list;
> +    int ret;
>      FsMountList mounts;
> -    struct FsMount *mount;
>      Error *local_err = NULL;
> -    int fd;
>
>      slog("guest-fsfreeze called");
>
> @@ -1740,122 +1571,34 @@ int64_t qmp_guest_fsfreeze_freeze_list(bool
> has_mountpoints,
>      /* cannot risk guest agent blocking itself on a write in this state */
>      ga_set_frozen(ga_state);
>
> -    QTAILQ_FOREACH_REVERSE(mount, &mounts, next) {
> -        /* To issue fsfreeze in the reverse order of mounts, check if the
> -         * mount is listed in the list here */
> -        if (has_mountpoints) {
> -            for (list = mountpoints; list; list = list->next) {
> -                if (strcmp(list->value, mount->dirname) == 0) {
> -                    break;
> -                }
> -            }
> -            if (!list) {
> -                continue;
> -            }
> -        }
> -
> -        fd = qga_open_cloexec(mount->dirname, O_RDONLY, 0);
> -        if (fd == -1) {
> -            error_setg_errno(errp, errno, "failed to open %s",
> mount->dirname);
> -            goto error;
> -        }
> -
> -        /* we try to cull filesystems we know won't work in advance, but
> other
> -         * filesystems may not implement fsfreeze for less obvious
> reasons.
> -         * these will report EOPNOTSUPP. we simply ignore these when
> tallying
> -         * the number of frozen filesystems.
> -         * if a filesystem is mounted more than once (aka bind mount) a
> -         * consecutive attempt to freeze an already frozen filesystem will
> -         * return EBUSY.
> -         *
> -         * any other error means a failure to freeze a filesystem we
> -         * expect to be freezable, so return an error in those cases
> -         * and return system to thawed state.
> -         */
> -        ret = ioctl(fd, FIFREEZE);
> -        if (ret == -1) {
> -            if (errno != EOPNOTSUPP && errno != EBUSY) {
> -                error_setg_errno(errp, errno, "failed to freeze %s",
> -                                 mount->dirname);
> -                close(fd);
> -                goto error;
> -            }
> -        } else {
> -            i++;
> -        }
> -        close(fd);
> -    }
> +    ret = qmp_guest_fsfreeze_do_freeze_list(has_mountpoints, mountpoints,
> +                                            mounts, errp);
>
>      free_fs_mount_list(&mounts);
>      /* We may not issue any FIFREEZE here.
>       * Just unset ga_state here and ready for the next call.
>       */
> -    if (i == 0) {
> +    if (ret == 0) {
>          ga_unset_frozen(ga_state);
> +    } else if (ret < 0) {
> +        qmp_guest_fsfreeze_thaw(NULL);
>      }
> -    return i;
> -
> -error:
> -    free_fs_mount_list(&mounts);
> -    qmp_guest_fsfreeze_thaw(NULL);
> -    return 0;
> +    return ret;
>  }
>
> -/*
> - * Walk list of frozen file systems in the guest, and thaw them.
> - */
>  int64_t qmp_guest_fsfreeze_thaw(Error **errp)
>  {
>      int ret;
> -    FsMountList mounts;
> -    FsMount *mount;
> -    int fd, i = 0, logged;
> -    Error *local_err = NULL;
>
> -    QTAILQ_INIT(&mounts);
> -    if (!build_fs_mount_list(&mounts, &local_err)) {
> -        error_propagate(errp, local_err);
> -        return 0;
> +    ret = qmp_guest_fsfreeze_do_thaw(errp);
> +    if (ret >= 0) {
> +        ga_unset_frozen(ga_state);
> +        execute_fsfreeze_hook(FSFREEZE_HOOK_THAW, errp);
> +    } else {
> +        ret = 0;
>      }
>
> -    QTAILQ_FOREACH(mount, &mounts, next) {
> -        logged = false;
> -        fd = qga_open_cloexec(mount->dirname, O_RDONLY, 0);
> -        if (fd == -1) {
> -            continue;
> -        }
> -        /* we have no way of knowing whether a filesystem was actually
> unfrozen
> -         * as a result of a successful call to FITHAW, only that if an
> error
> -         * was returned the filesystem was *not* unfrozen by that
> particular
> -         * call.
> -         *
> -         * since multiple preceding FIFREEZEs require multiple calls to
> FITHAW
> -         * to unfreeze, continuing issuing FITHAW until an error is
> returned,
> -         * in which case either the filesystem is in an unfreezable
> state, or,
> -         * more likely, it was thawed previously (and remains so
> afterward).
> -         *
> -         * also, since the most recent successful call is the one that did
> -         * the actual unfreeze, we can use this to provide an accurate
> count
> -         * of the number of filesystems unfrozen by guest-fsfreeze-thaw,
> which
> -         * may * be useful for determining whether a filesystem was
> unfrozen
> -         * during the freeze/thaw phase by a process other than qemu-ga.
> -         */
> -        do {
> -            ret = ioctl(fd, FITHAW);
> -            if (ret == 0 && !logged) {
> -                i++;
> -                logged = true;
> -            }
> -        } while (ret == 0);
> -        close(fd);
> -    }
> -
> -    ga_unset_frozen(ga_state);
> -    free_fs_mount_list(&mounts);
> -
> -    execute_fsfreeze_hook(FSFREEZE_HOOK_THAW, errp);
> -
> -    return i;
> +    return ret;
>  }
>
>  static void guest_fsfreeze_cleanup(void)
> diff --git a/qga/meson.build b/qga/meson.build
> index 65c1e93846..409f49a000 100644
> --- a/qga/meson.build
> +++ b/qga/meson.build
> @@ -72,6 +72,9 @@ qga_ss.add(when: 'CONFIG_POSIX', if_true: files(
>    'commands-posix.c',
>    'commands-posix-ssh.c',
>  ))
> +qga_ss.add(when: 'CONFIG_LINUX', if_true: files(
> +  'commands-linux.c',
> +))
>  qga_ss.add(when: 'CONFIG_WIN32', if_true: files(
>    'channel-win32.c',
>    'commands-win32.c',
> --
> 2.34.1
>
>
>

-- 
Marc-André Lureau

[-- Attachment #2: Type: text/html, Size: 31020 bytes --]

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 1/7] qga: Add initial FreeBSD support
  2022-09-29  7:52 ` [PATCH 1/7] qga: Add initial " Alexander Ivanov
@ 2022-09-29 11:29   ` Marc-André Lureau
  2022-09-29 14:11     ` Alexander Ivanov
  0 siblings, 1 reply; 20+ messages in thread
From: Marc-André Lureau @ 2022-09-29 11:29 UTC (permalink / raw)
  To: Alexander Ivanov; +Cc: qemu-devel, den, michael.roth, kkostiuk

[-- Attachment #1: Type: text/plain, Size: 3398 bytes --]

On Thu, Sep 29, 2022 at 11:56 AM Alexander Ivanov <
alexander.ivanov@virtuozzo.com> wrote:

> - Fix device path.
> - Fix virtio-serial channel initialization.
> - Make the code buildable in FreeBSD.
>
> Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
> ---
>  meson.build          |  2 +-
>  qga/channel-posix.c  | 14 ++++++++++++++
>  qga/commands-posix.c |  8 ++++++++
>  qga/main.c           |  6 +++++-
>  4 files changed, 28 insertions(+), 2 deletions(-)
>
> diff --git a/meson.build b/meson.build
> index 8dc661363f..5c11abc8aa 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -75,7 +75,7 @@ have_tools = get_option('tools') \
>    .allowed()
>  have_ga = get_option('guest_agent') \
>    .disable_auto_if(not have_system and not have_tools) \
> -  .require(targetos in ['sunos', 'linux', 'windows'],
> +  .require(targetos in ['sunos', 'linux', 'windows', 'freebsd'],
>             error_message: 'unsupported OS for QEMU guest agent') \
>    .allowed()
>  have_block = have_system or have_tools
> diff --git a/qga/channel-posix.c b/qga/channel-posix.c
> index 6796a02cff..0f14246563 100644
> --- a/qga/channel-posix.c
> +++ b/qga/channel-posix.c
> @@ -149,6 +149,20 @@ static gboolean ga_channel_open(GAChannel *c, const
> gchar *path,
>              return false;
>          }
>  #endif
> +#ifdef __FreeBSD__
> +        struct termios tio;
> +        if (tcgetattr(fd, &tio) < 0) {
> +            error_setg_errno(errp, errno, "error getting channel termios
> attrs");
> +            close(fd);
> +            return false;
> +        }
> +        tio.c_lflag &= ~ECHO;
> +        if (tcsetattr(fd, TCSAFLUSH, &tio) < 0) {
> +            error_setg_errno(errp, errno, "error setting channel termios
> attrs");
> +            close(fd);
> +            return false;
> +        }
> +#endif /* __FreeBSD__ */
>

It could help to document why this is needed. I assume this is correct, so:

Acked-by: Marc-André Lureau <marcandre.lureau@redhat.com>




>          ret = ga_channel_client_add(c, fd);
>          if (ret) {
>              error_setg(errp, "error adding channel to main loop");
> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> index eea819cff0..16d67e9f6d 100644
> --- a/qga/commands-posix.c
> +++ b/qga/commands-posix.c
> @@ -51,6 +51,14 @@
>  #endif
>  #endif
>
> +#ifdef __FreeBSD__
> +/*
> + * The code under HAVE_GETIFADDRS condition can't be compiled in FreeBSD.
> + * Fix it in one of the following patches.
> + */
> +#undef HAVE_GETIFADDRS
> +#endif
> +
>  #ifdef HAVE_GETIFADDRS
>  #include <arpa/inet.h>
>  #include <sys/socket.h>
> diff --git a/qga/main.c b/qga/main.c
> index 5a9d8252e0..0d27c97d38 100644
> --- a/qga/main.c
> +++ b/qga/main.c
> @@ -45,9 +45,13 @@
>  #endif
>
>  #ifndef _WIN32
> +#ifdef __FreeBSD__
> +#define QGA_VIRTIO_PATH_DEFAULT "/dev/vtcon/org.qemu.guest_agent.0"
> +#else /* __FreeBSD__ */
>  #define QGA_VIRTIO_PATH_DEFAULT "/dev/virtio-ports/org.qemu.guest_agent.0"
> -#define QGA_STATE_RELATIVE_DIR  "run"
> +#endif /* __FreeBSD__ */
>  #define QGA_SERIAL_PATH_DEFAULT "/dev/ttyS0"
> +#define QGA_STATE_RELATIVE_DIR  "run"
>  #else
>  #define QGA_VIRTIO_PATH_DEFAULT "\\\\.\\Global\\org.qemu.guest_agent.0"
>  #define QGA_STATE_RELATIVE_DIR  "qemu-ga"
> --
> 2.34.1
>
>
>

-- 
Marc-André Lureau

[-- Attachment #2: Type: text/html, Size: 4645 bytes --]

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 1/7] qga: Add initial FreeBSD support
  2022-09-29 11:29   ` Marc-André Lureau
@ 2022-09-29 14:11     ` Alexander Ivanov
  0 siblings, 0 replies; 20+ messages in thread
From: Alexander Ivanov @ 2022-09-29 14:11 UTC (permalink / raw)
  To: Marc-André Lureau; +Cc: qemu-devel, den, michael.roth, kkostiuk

Hi Marc-André,

Thank you for the review.

On 29.09.2022 13:29, Marc-André Lureau wrote:
>
>
> On Thu, Sep 29, 2022 at 11:56 AM Alexander Ivanov 
> <alexander.ivanov@virtuozzo.com> wrote:
>
>     - Fix device path.
>     - Fix virtio-serial channel initialization.
>     - Make the code buildable in FreeBSD.
>
>     Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
>     ---
>      meson.build          |  2 +-
>      qga/channel-posix.c  | 14 ++++++++++++++
>      qga/commands-posix.c |  8 ++++++++
>      qga/main.c           |  6 +++++-
>      4 files changed, 28 insertions(+), 2 deletions(-)
>
>     diff --git a/meson.build b/meson.build
>     index 8dc661363f..5c11abc8aa 100644
>     --- a/meson.build
>     +++ b/meson.build
>     @@ -75,7 +75,7 @@ have_tools = get_option('tools') \
>        .allowed()
>      have_ga = get_option('guest_agent') \
>        .disable_auto_if(not have_system and not have_tools) \
>     -  .require(targetos in ['sunos', 'linux', 'windows'],
>     +  .require(targetos in ['sunos', 'linux', 'windows', 'freebsd'],
>                 error_message: 'unsupported OS for QEMU guest agent') \
>        .allowed()
>      have_block = have_system or have_tools
>     diff --git a/qga/channel-posix.c b/qga/channel-posix.c
>     index 6796a02cff..0f14246563 100644
>     --- a/qga/channel-posix.c
>     +++ b/qga/channel-posix.c
>     @@ -149,6 +149,20 @@ static gboolean ga_channel_open(GAChannel *c,
>     const gchar *path,
>                  return false;
>              }
>      #endif
>     +#ifdef __FreeBSD__
>     +        struct termios tio;
>     +        if (tcgetattr(fd, &tio) < 0) {
>     +            error_setg_errno(errp, errno, "error getting channel
>     termios attrs");
>     +            close(fd);
>     +            return false;
>     +        }
>     +        tio.c_lflag &= ~ECHO;
>     +        if (tcsetattr(fd, TCSAFLUSH, &tio) < 0) {
>     +            error_setg_errno(errp, errno, "error setting channel
>     termios attrs");
>     +            close(fd);
>     +            return false;
>     +        }
>     +#endif /* __FreeBSD__ */
>
It is for echo suppressing. In other way a host client gets in return 
the commands it sends and raises an error. Will add a comment.

> It could help to document why this is needed. I assume this is 
> correct, so:
>
> Acked-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>
>
>
>              ret = ga_channel_client_add(c, fd);
>              if (ret) {
>                  error_setg(errp, "error adding channel to main loop");
>     diff --git a/qga/commands-posix.c b/qga/commands-posix.c
>     index eea819cff0..16d67e9f6d 100644
>     --- a/qga/commands-posix.c
>     +++ b/qga/commands-posix.c
>     @@ -51,6 +51,14 @@
>      #endif
>      #endif
>
>     +#ifdef __FreeBSD__
>     +/*
>     + * The code under HAVE_GETIFADDRS condition can't be compiled in
>     FreeBSD.
>     + * Fix it in one of the following patches.
>     + */
>     +#undef HAVE_GETIFADDRS
>     +#endif
>     +
>      #ifdef HAVE_GETIFADDRS
>      #include <arpa/inet.h>
>      #include <sys/socket.h>
>     diff --git a/qga/main.c b/qga/main.c
>     index 5a9d8252e0..0d27c97d38 100644
>     --- a/qga/main.c
>     +++ b/qga/main.c
>     @@ -45,9 +45,13 @@
>      #endif
>
>      #ifndef _WIN32
>     +#ifdef __FreeBSD__
>     +#define QGA_VIRTIO_PATH_DEFAULT "/dev/vtcon/org.qemu.guest_agent.0"
>     +#else /* __FreeBSD__ */
>      #define QGA_VIRTIO_PATH_DEFAULT
>     "/dev/virtio-ports/org.qemu.guest_agent.0"
>     -#define QGA_STATE_RELATIVE_DIR  "run"
>     +#endif /* __FreeBSD__ */
>      #define QGA_SERIAL_PATH_DEFAULT "/dev/ttyS0"
>     +#define QGA_STATE_RELATIVE_DIR  "run"
>      #else
>      #define QGA_VIRTIO_PATH_DEFAULT
>     "\\\\.\\Global\\org.qemu.guest_agent.0"
>      #define QGA_STATE_RELATIVE_DIR  "qemu-ga"
>     -- 
>     2.34.1
>
>
>
>
> -- 
> Marc-André Lureau


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 5/7] qga: Add support for user password setting in FreeBSD
  2022-09-29 11:22   ` Marc-André Lureau
@ 2022-09-29 14:29     ` Alexander Ivanov
  2022-09-30  8:19       ` Marc-André Lureau
  0 siblings, 1 reply; 20+ messages in thread
From: Alexander Ivanov @ 2022-09-29 14:29 UTC (permalink / raw)
  To: Marc-André Lureau; +Cc: qemu-devel, den, michael.roth, kkostiuk


On 29.09.2022 13:22, Marc-André Lureau wrote:
>
> 	
> Caution: This is an external email and has a suspicious subject or 
> content. Please take care when clicking links or opening attachments. 
> When in doubt, contact your IT Department
>
>
> Hi Alexander
>
> On Thu, Sep 29, 2022 at 12:52 PM Alexander Ivanov 
> <alexander.ivanov@virtuozzo.com> wrote:
>
>     Move qmp_guest_set_user_password() from __linux__ condition to
>     (__linux__ || __FreeBSD__) condition. Add command and arguments
>     for password setting in FreeBSD.
>
>     Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
>
>
> Could you explain why you need to move the code?
>
> You could instead have a top declaration?
Now qmp_guest_set_user_password()is under #ifdef __linux__. I want to 
use this function for FreeBSD too, so I moved it under (__linux__ || 
__FreeBSD__) condition. I could create another function for FreeBSD but 
it would lead to code duplication. Unfortunately I don't see another way 
to do it except to add (__linux__ || __FreeBSD__) condition, cut the 
function and paste in this condition. If you could suggest another way I 
would happy.
> If it's really required, please split the patch in move + additions.
Just moving the function without other changes leads to unbuildable code 
in FreeBSD. I thought it's worse than moving and changing at the same 
time. I split the patch if you prefer.
>
>     ---
>      qga/commands-posix.c | 223
>     +++++++++++++++++++++++--------------------
>      1 file changed, 118 insertions(+), 105 deletions(-)
>
>     diff --git a/qga/commands-posix.c b/qga/commands-posix.c
>     index 88e0d0fe24..6ce894ca6e 100644
>     --- a/qga/commands-posix.c
>     +++ b/qga/commands-posix.c
>     @@ -2123,111 +2123,6 @@ int64_t
>     qmp_guest_set_vcpus(GuestLogicalProcessorList *vcpus, Error **errp)
>          return processed;
>      }
>
>     -void qmp_guest_set_user_password(const char *username,
>     -                                 const char *password,
>     -                                 bool crypted,
>     -                                 Error **errp)
>     -{
>     -    Error *local_err = NULL;
>     -    char *passwd_path = NULL;
>     -    pid_t pid;
>     -    int status;
>     -    int datafd[2] = { -1, -1 };
>     -    char *rawpasswddata = NULL;
>     -    size_t rawpasswdlen;
>     -    char *chpasswddata = NULL;
>     -    size_t chpasswdlen;
>     -
>     -    rawpasswddata = (char *)qbase64_decode(password, -1,
>     &rawpasswdlen, errp);
>     -    if (!rawpasswddata) {
>     -        return;
>     -    }
>     -    rawpasswddata = g_renew(char, rawpasswddata, rawpasswdlen + 1);
>     -    rawpasswddata[rawpasswdlen] = '\0';
>     -
>     -    if (strchr(rawpasswddata, '\n')) {
>     -        error_setg(errp, "forbidden characters in raw password");
>     -        goto out;
>     -    }
>     -
>     -    if (strchr(username, '\n') ||
>     -        strchr(username, ':')) {
>     -        error_setg(errp, "forbidden characters in username");
>     -        goto out;
>     -    }
>     -
>     -    chpasswddata = g_strdup_printf("%s:%s\n", username,
>     rawpasswddata);
>     -    chpasswdlen = strlen(chpasswddata);
>     -
>     -    passwd_path = g_find_program_in_path("chpasswd");
>     -
>     -    if (!passwd_path) {
>     -        error_setg(errp, "cannot find 'passwd' program in PATH");
>     -        goto out;
>     -    }
>     -
>     -    if (!g_unix_open_pipe(datafd, FD_CLOEXEC, NULL)) {
>     -        error_setg(errp, "cannot create pipe FDs");
>     -        goto out;
>     -    }
>     -
>     -    pid = fork();
>     -    if (pid == 0) {
>     -        close(datafd[1]);
>     -        /* child */
>     -        setsid();
>     -        dup2(datafd[0], 0);
>     -        reopen_fd_to_null(1);
>     -        reopen_fd_to_null(2);
>     -
>     -        if (crypted) {
>     -            execl(passwd_path, "chpasswd", "-e", NULL);
>     -        } else {
>     -            execl(passwd_path, "chpasswd", NULL);
>     -        }
>     -        _exit(EXIT_FAILURE);
>     -    } else if (pid < 0) {
>     -        error_setg_errno(errp, errno, "failed to create child
>     process");
>     -        goto out;
>     -    }
>     -    close(datafd[0]);
>     -    datafd[0] = -1;
>     -
>     -    if (qemu_write_full(datafd[1], chpasswddata, chpasswdlen) !=
>     chpasswdlen) {
>     -        error_setg_errno(errp, errno, "cannot write new account
>     password");
>     -        goto out;
>     -    }
>     -    close(datafd[1]);
>     -    datafd[1] = -1;
>     -
>     -    ga_wait_child(pid, &status, &local_err);
>     -    if (local_err) {
>     -        error_propagate(errp, local_err);
>     -        goto out;
>     -    }
>     -
>     -    if (!WIFEXITED(status)) {
>     -        error_setg(errp, "child process has terminated abnormally");
>     -        goto out;
>     -    }
>     -
>     -    if (WEXITSTATUS(status)) {
>     -        error_setg(errp, "child process has failed to set user
>     password");
>     -        goto out;
>     -    }
>     -
>     -out:
>     -    g_free(chpasswddata);
>     -    g_free(rawpasswddata);
>     -    g_free(passwd_path);
>     -    if (datafd[0] != -1) {
>     -        close(datafd[0]);
>     -    }
>     -    if (datafd[1] != -1) {
>     -        close(datafd[1]);
>     -    }
>     -}
>     -
>      static void ga_read_sysfs_file(int dirfd, const char *pathname,
>     char *buf,
>                                     int size, Error **errp)
>      {
>     @@ -2793,6 +2688,124 @@ GuestMemoryBlockInfo
>     *qmp_guest_get_memory_block_info(Error **errp)
>
>      #endif
>
>     +#if defined(__linux__) || defined(__FreeBSD__)
>     +void qmp_guest_set_user_password(const char *username,
>     +                                 const char *password,
>     +                                 bool crypted,
>     +                                 Error **errp)
>     +{
>     +    Error *local_err = NULL;
>     +    char *passwd_path = NULL;
>     +    pid_t pid;
>     +    int status;
>     +    int datafd[2] = { -1, -1 };
>     +    char *rawpasswddata = NULL;
>     +    size_t rawpasswdlen;
>     +    char *chpasswddata = NULL;
>     +    size_t chpasswdlen;
>     +
>     +    rawpasswddata = (char *)qbase64_decode(password, -1,
>     &rawpasswdlen, errp);
>     +    if (!rawpasswddata) {
>     +        return;
>     +    }
>     +    rawpasswddata = g_renew(char, rawpasswddata, rawpasswdlen + 1);
>     +    rawpasswddata[rawpasswdlen] = '\0';
>     +
>     +    if (strchr(rawpasswddata, '\n')) {
>     +        error_setg(errp, "forbidden characters in raw password");
>     +        goto out;
>     +    }
>     +
>     +    if (strchr(username, '\n') ||
>     +        strchr(username, ':')) {
>     +        error_setg(errp, "forbidden characters in username");
>     +        goto out;
>     +    }
>     +
>     +#ifdef __FreeBSD__
>     +    chpasswddata = g_strdup(rawpasswddata);
>     +    passwd_path = g_find_program_in_path("pw");
>     +#else
>     +    chpasswddata = g_strdup_printf("%s:%s\n", username,
>     rawpasswddata);
>     +    passwd_path = g_find_program_in_path("chpasswd");
>     +#endif
>     +
>     +    chpasswdlen = strlen(chpasswddata);
>     +
>     +    if (!passwd_path) {
>     +        error_setg(errp, "cannot find 'passwd' program in PATH");
>     +        goto out;
>     +    }
>     +
>     +    if (!g_unix_open_pipe(datafd, FD_CLOEXEC, NULL)) {
>     +        error_setg(errp, "cannot create pipe FDs");
>     +        goto out;
>     +    }
>     +
>     +    pid = fork();
>     +    if (pid == 0) {
>     +        close(datafd[1]);
>     +        /* child */
>     +        setsid();
>     +        dup2(datafd[0], 0);
>     +        reopen_fd_to_null(1);
>     +        reopen_fd_to_null(2);
>     +
>     +#ifdef __FreeBSD__
>     +        const char *h_arg;
>     +        h_arg = (crypted) ? "-H" : "-h";
>     +        execl(passwd_path, "pw", "usermod", "-n", username,
>     h_arg, "0", NULL);
>     +#else
>     +        if (crypted) {
>     +            execl(passwd_path, "chpasswd", "-e", NULL);
>     +        } else {
>     +            execl(passwd_path, "chpasswd", NULL);
>     +        }
>     +#endif
>     +        _exit(EXIT_FAILURE);
>     +    } else if (pid < 0) {
>     +        error_setg_errno(errp, errno, "failed to create child
>     process");
>     +        goto out;
>     +    }
>     +    close(datafd[0]);
>     +    datafd[0] = -1;
>     +
>     +    if (qemu_write_full(datafd[1], chpasswddata, chpasswdlen) !=
>     chpasswdlen) {
>     +        error_setg_errno(errp, errno, "cannot write new account
>     password");
>     +        goto out;
>     +    }
>     +    close(datafd[1]);
>     +    datafd[1] = -1;
>     +
>     +    ga_wait_child(pid, &status, &local_err);
>     +    if (local_err) {
>     +        error_propagate(errp, local_err);
>     +        goto out;
>     +    }
>     +
>     +    if (!WIFEXITED(status)) {
>     +        error_setg(errp, "child process has terminated abnormally");
>     +        goto out;
>     +    }
>     +
>     +    if (WEXITSTATUS(status)) {
>     +        error_setg(errp, "child process has failed to set user
>     password");
>     +        goto out;
>     +    }
>     +
>     +out:
>     +    g_free(chpasswddata);
>     +    g_free(rawpasswddata);
>     +    g_free(passwd_path);
>     +    if (datafd[0] != -1) {
>     +        close(datafd[0]);
>     +    }
>     +    if (datafd[1] != -1) {
>     +        close(datafd[1]);
>     +    }
>     +}
>     +#endif
>     +
>      #ifdef HAVE_GETIFADDRS
>      static GuestNetworkInterface *
>      guest_find_interface(GuestNetworkInterfaceList *head,
>     -- 
>     2.34.1
>
>
>
>
> -- 
> Marc-André Lureau


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 6/7] qga: Move HW address getting to a separate function
  2022-09-29 11:28   ` Marc-André Lureau
@ 2022-09-29 14:33     ` Alexander Ivanov
  0 siblings, 0 replies; 20+ messages in thread
From: Alexander Ivanov @ 2022-09-29 14:33 UTC (permalink / raw)
  To: Marc-André Lureau; +Cc: qemu-devel, den, michael.roth, kkostiuk


On 29.09.2022 13:28, Marc-André Lureau wrote:
> Hi
>
> On Thu, Sep 29, 2022 at 12:02 PM Alexander Ivanov 
> <alexander.ivanov@virtuozzo.com> wrote:
>
>     In the next patch FreeBSD support for guest-network-get-interfaces
>     will be
>     added. Previously move Linux-specific code of HW address getting to a
>     separate functions and add a dumb function to commands-bsd.c.
>
>     Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
>     ---
>      qga/commands-bsd.c    |  18 +++++++
>      qga/commands-common.h |   6 +++
>      qga/commands-posix.c  | 114
>     +++++++++++++++++++++++-------------------
>      3 files changed, 87 insertions(+), 51 deletions(-)
>
>     diff --git a/qga/commands-bsd.c b/qga/commands-bsd.c
>     index ca06692179..ca81114171 100644
>     --- a/qga/commands-bsd.c
>     +++ b/qga/commands-bsd.c
>     @@ -167,3 +167,21 @@ GuestCpuStatsList
>     *qmp_guest_get_cpustats(Error **errp)
>          return NULL;
>      }
>      #endif /* CONFIG_FSFREEZE */
>     +
>     +#ifdef HAVE_GETIFADDRS
>     +/*
>     + * Fill buf with MAC address by ifaddrs. Pointer buf must point to a
>     + * buffer with ETHER_ADDR_LEN length at least.
>     + *
>     + * Returns -1 in case of an error, 0 if MAC address can't be
>     obtained or
>     + * 1 if MAC addres is obtained.
>
>
> Not a typical Error function return value...
>
> Eventually, you could return a bool for error/ok and take an 
> additional "bool *obtained/valid" argument. Just a suggestion.
Got it.
>
>     + */
>     +int guest_get_hw_addr(struct ifaddrs *ifa, unsigned char *buf,
>     Error **errp)
>     +{
>     +    /*
>     +     * We can't get the hw addr of this interface,
>     +     * but that's not a fatal error.
>     +     */
>     +    return 0;
>     +}
>     +#endif /* HAVE_GETIFADDRS */
>     diff --git a/qga/commands-common.h b/qga/commands-common.h
>     index 2d9878a634..2485a037fd 100644
>     --- a/qga/commands-common.h
>     +++ b/qga/commands-common.h
>     @@ -56,6 +56,12 @@ int64_t qmp_guest_fsfreeze_do_freeze_list(bool
>     has_mountpoints,
>      int qmp_guest_fsfreeze_do_thaw(Error **errp);
>      #endif /* CONFIG_FSFREEZE */
>
>     +#ifdef HAVE_GETIFADDRS
>     +#include <ifaddrs.h>
>     +
>     +int guest_get_hw_addr(struct ifaddrs *ifa, unsigned char *buf,
>     Error **errp);
>     +#endif
>     +
>      typedef struct GuestFileHandle GuestFileHandle;
>
>      GuestFileHandle *guest_file_handle_find(int64_t id, Error **errp);
>     diff --git a/qga/commands-posix.c b/qga/commands-posix.c
>     index 6ce894ca6e..0bebd9e690 100644
>     --- a/qga/commands-posix.c
>     +++ b/qga/commands-posix.c
>     @@ -41,20 +41,12 @@
>      #endif
>      #endif
>
>     -#ifdef __FreeBSD__
>     -/*
>     - * The code under HAVE_GETIFADDRS condition can't be compiled in
>     FreeBSD.
>     - * Fix it in one of the following patches.
>     - */
>     -#undef HAVE_GETIFADDRS
>     -#endif
>     -
>      #ifdef HAVE_GETIFADDRS
>      #include <arpa/inet.h>
>      #include <sys/socket.h>
>      #include <net/if.h>
>     +#include <net/ethernet.h>
>      #include <sys/types.h>
>     -#include <ifaddrs.h>
>      #ifdef CONFIG_SOLARIS
>      #include <sys/sockio.h>
>      #endif
>     @@ -2659,14 +2651,6 @@ int64_t
>     qmp_guest_set_vcpus(GuestLogicalProcessorList *vcpus, Error **errp)
>          return -1;
>      }
>
>     -void qmp_guest_set_user_password(const char *username,
>     -                                 const char *password,
>     -                                 bool crypted,
>     -                                 Error **errp)
>     -{
>     -    error_setg(errp, QERR_UNSUPPORTED);
>     -}
>     -
>
>
> Why this in this patch?
Something went wrong when I re-created patches. My bad. Will fix it.
>
>      GuestMemoryBlockList *qmp_guest_get_memory_blocks(Error **errp)
>      {
>          error_setg(errp, QERR_UNSUPPORTED);
>     @@ -2804,7 +2788,15 @@ out:
>              close(datafd[1]);
>          }
>      }
>     -#endif
>     +#else /* __linux__ || __FreeBSD__ */
>     +void qmp_guest_set_user_password(const char *username,
>     +                                 const char *password,
>     +                                 bool crypted,
>     +                                 Error **errp)
>     +{
>     +    error_setg(errp, QERR_UNSUPPORTED);
>     +}
>     +#endif /* __linux__ || __FreeBSD__ */
>
>      #ifdef HAVE_GETIFADDRS
>      static GuestNetworkInterface *
>     @@ -2887,6 +2879,54 @@ static int guest_get_network_stats(const
>     char *name,
>          return -1;
>      }
>
>     +#ifndef __FreeBSD__
>     +/*
>     + * Fill buf with MAC address by ifaddrs. Pointer buf must point to a
>     + * buffer with ETHER_ADDR_LEN length at least.
>     + * Returns -1 in case of an error, 0 if MAC address can't be
>     obtained or
>     + * 1 if MAC addres is obtained.
>     + */
>     +int guest_get_hw_addr(struct ifaddrs *ifa, unsigned char *buf,
>     Error **errp)
>     +{
>     +    struct ifreq ifr;
>     +    int sock;
>     +
>     +    /* we haven't obtained HW address yet */
>     +    sock = socket(PF_INET, SOCK_STREAM, 0);
>     +    if (sock == -1) {
>     +        error_setg_errno(errp, errno, "failed to create socket");
>     +        return -1;
>     +    }
>     +
>     +    memset(&ifr, 0, sizeof(ifr));
>     +    pstrcpy(ifr.ifr_name, IF_NAMESIZE, ifa->ifa_name);
>     +    if (ioctl(sock, SIOCGIFHWADDR, &ifr) == -1) {
>     +        /*
>     +         * We can't get the hw addr of this interface, but that's
>     not a
>     +         * fatal error. Don't set info->hardware_address, but keep
>     +         * going.
>     +         */
>     +        if (errno == EADDRNOTAVAIL) {
>     +            /* The interface doesn't have a hw addr (e.g.
>     loopback). */
>     +            g_debug("failed to get MAC address of %s: %s",
>     +                    ifa->ifa_name, strerror(errno));
>     +        } else{
>     +            g_warning("failed to get MAC address of %s: %s",
>     +                      ifa->ifa_name, strerror(errno));
>     +        }
>     +        close(sock);
>     +        return 0;
>     +    }
>     +#ifdef CONFIG_SOLARIS
>     +    memcpy(buf, &ifr.ifr_addr.sa_data, ETHER_ADDR_LEN);
>     +#else
>     +    memcpy(buf, &ifr.ifr_hwaddr.sa_data, ETHER_ADDR_LEN);
>     +#endif
>     +    close(sock);
>     +    return 1;
>     +}
>     +#endif /* __FreeBSD__ */
>     +
>      /*
>       * Build information about guest interfaces
>       */
>     @@ -2907,10 +2947,9 @@ GuestNetworkInterfaceList
>     *qmp_guest_network_get_interfaces(Error **errp)
>              GuestNetworkInterfaceStat *interface_stat = NULL;
>              char addr4[INET_ADDRSTRLEN];
>              char addr6[INET6_ADDRSTRLEN];
>     -        int sock;
>     -        struct ifreq ifr;
>     -        unsigned char *mac_addr;
>     +        unsigned char mac_addr[ETHER_ADDR_LEN];
>              void *p;
>     +        int ret;
>
>              g_debug("Processing %s interface", ifa->ifa_name);
>
>     @@ -2924,45 +2963,18 @@ GuestNetworkInterfaceList
>     *qmp_guest_network_get_interfaces(Error **errp)
>              }
>
>              if (!info->has_hardware_address) {
>     -            /* we haven't obtained HW address yet */
>     -            sock = socket(PF_INET, SOCK_STREAM, 0);
>     -            if (sock == -1) {
>     -                error_setg_errno(errp, errno, "failed to create
>     socket");
>     +            ret = guest_get_hw_addr(ifa, mac_addr, errp);
>     +            if (ret == -1) {
>                      goto error;
>                  }
>     -
>     -            memset(&ifr, 0, sizeof(ifr));
>     -            pstrcpy(ifr.ifr_name, IF_NAMESIZE, info->name);
>     -            if (ioctl(sock, SIOCGIFHWADDR, &ifr) == -1) {
>     -                /*
>     -                 * We can't get the hw addr of this interface,
>     but that's not a
>     -                 * fatal error. Don't set info->hardware_address,
>     but keep
>     -                 * going.
>     -                 */
>     -                if (errno == EADDRNOTAVAIL) {
>     -                    /* The interface doesn't have a hw addr (e.g.
>     loopback). */
>     -                    g_debug("failed to get MAC address of %s: %s",
>     -                            ifa->ifa_name, strerror(errno));
>     -                } else{
>     -                    g_warning("failed to get MAC address of %s: %s",
>     -                              ifa->ifa_name, strerror(errno));
>     -                }
>     -
>     -            } else {
>     -#ifdef CONFIG_SOLARIS
>     -                mac_addr = (unsigned char *) &ifr.ifr_addr.sa_data;
>     -#else
>     -                mac_addr = (unsigned char *) &ifr.ifr_hwaddr.sa_data;
>     -#endif
>     +            if (ret == 1) {
>                      info->hardware_address =
>      g_strdup_printf("%02x:%02x:%02x:%02x:%02x:%02x",
>                                          (int) mac_addr[0], (int)
>     mac_addr[1],
>                                          (int) mac_addr[2], (int)
>     mac_addr[3],
>                                          (int) mac_addr[4], (int)
>     mac_addr[5]);
>     -
>                      info->has_hardware_address = true;
>                  }
>     -            close(sock);
>              }
>
>              if (ifa->ifa_addr &&
>     -- 
>     2.34.1
>
>
>
> looks ok to me otherwise
>
> -- 
> Marc-André Lureau


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 5/7] qga: Add support for user password setting in FreeBSD
  2022-09-29 14:29     ` Alexander Ivanov
@ 2022-09-30  8:19       ` Marc-André Lureau
  2022-09-30 13:11         ` Alexander Ivanov
  0 siblings, 1 reply; 20+ messages in thread
From: Marc-André Lureau @ 2022-09-30  8:19 UTC (permalink / raw)
  To: Alexander Ivanov; +Cc: qemu-devel, den, michael.roth, kkostiuk

[-- Attachment #1: Type: text/plain, Size: 11684 bytes --]

Hi

On Thu, Sep 29, 2022 at 6:29 PM Alexander Ivanov <
alexander.ivanov@virtuozzo.com> wrote:

>
> On 29.09.2022 13:22, Marc-André Lureau wrote:
> >
> >
> > Caution: This is an external email and has a suspicious subject or
> > content. Please take care when clicking links or opening attachments.
> > When in doubt, contact your IT Department
> >
> >
> > Hi Alexander
> >
> > On Thu, Sep 29, 2022 at 12:52 PM Alexander Ivanov
> > <alexander.ivanov@virtuozzo.com> wrote:
> >
> >     Move qmp_guest_set_user_password() from __linux__ condition to
> >     (__linux__ || __FreeBSD__) condition. Add command and arguments
> >     for password setting in FreeBSD.
> >
> >     Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
> >
> >
> > Could you explain why you need to move the code?
> >
> > You could instead have a top declaration?
> Now qmp_guest_set_user_password()is under #ifdef __linux__. I want to
> use this function for FreeBSD too, so I moved it under (__linux__ ||
> __FreeBSD__) condition. I could create another function for FreeBSD but
>

Why not make the following change?

diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index 88e0d0fe24..78a345c9f3 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -2122,7 +2122,9 @@ int64_t qmp_guest_set_vcpus(GuestLogicalProcessorList
*vcpus, Error **errp)

     return processed;
 }
+#endif /* linux */

+#if defined(__linux__) || defined(__FreeBSD__)
 void qmp_guest_set_user_password(const char *username,
                                  const char *password,
                                  bool crypted,
@@ -2227,7 +2229,9 @@ out:
         close(datafd[1]);
     }
 }
+#endif /* linux || freebsd */

+#if defined(__linux__)
 static void ga_read_sysfs_file(int dirfd, const char *pathname, char *buf,
                                int size, Error **errp)
 {


> it would lead to code duplication. Unfortunately I don't see another way
> to do it except to add (__linux__ || __FreeBSD__) condition, cut the
> function and paste in this condition. If you could suggest another way I
> would happy.
> > If it's really required, please split the patch in move + additions.
> Just moving the function without other changes leads to unbuildable code
> in FreeBSD. I thought it's worse than moving and changing at the same
> time. I split the patch if you prefer.
>
>
> >     ---
> >      qga/commands-posix.c | 223
> >     +++++++++++++++++++++++--------------------
> >      1 file changed, 118 insertions(+), 105 deletions(-)
> >
> >     diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> >     index 88e0d0fe24..6ce894ca6e 100644
> >     --- a/qga/commands-posix.c
> >     +++ b/qga/commands-posix.c
> >     @@ -2123,111 +2123,6 @@ int64_t
> >     qmp_guest_set_vcpus(GuestLogicalProcessorList *vcpus, Error **errp)
> >          return processed;
> >      }
> >
> >     -void qmp_guest_set_user_password(const char *username,
> >     -                                 const char *password,
> >     -                                 bool crypted,
> >     -                                 Error **errp)
> >     -{
> >     -    Error *local_err = NULL;
> >     -    char *passwd_path = NULL;
> >     -    pid_t pid;
> >     -    int status;
> >     -    int datafd[2] = { -1, -1 };
> >     -    char *rawpasswddata = NULL;
> >     -    size_t rawpasswdlen;
> >     -    char *chpasswddata = NULL;
> >     -    size_t chpasswdlen;
> >     -
> >     -    rawpasswddata = (char *)qbase64_decode(password, -1,
> >     &rawpasswdlen, errp);
> >     -    if (!rawpasswddata) {
> >     -        return;
> >     -    }
> >     -    rawpasswddata = g_renew(char, rawpasswddata, rawpasswdlen + 1);
> >     -    rawpasswddata[rawpasswdlen] = '\0';
> >     -
> >     -    if (strchr(rawpasswddata, '\n')) {
> >     -        error_setg(errp, "forbidden characters in raw password");
> >     -        goto out;
> >     -    }
> >     -
> >     -    if (strchr(username, '\n') ||
> >     -        strchr(username, ':')) {
> >     -        error_setg(errp, "forbidden characters in username");
> >     -        goto out;
> >     -    }
> >     -
> >     -    chpasswddata = g_strdup_printf("%s:%s\n", username,
> >     rawpasswddata);
> >     -    chpasswdlen = strlen(chpasswddata);
> >     -
> >     -    passwd_path = g_find_program_in_path("chpasswd");
> >     -
> >     -    if (!passwd_path) {
> >     -        error_setg(errp, "cannot find 'passwd' program in PATH");
> >     -        goto out;
> >     -    }
> >     -
> >     -    if (!g_unix_open_pipe(datafd, FD_CLOEXEC, NULL)) {
> >     -        error_setg(errp, "cannot create pipe FDs");
> >     -        goto out;
> >     -    }
> >     -
> >     -    pid = fork();
> >     -    if (pid == 0) {
> >     -        close(datafd[1]);
> >     -        /* child */
> >     -        setsid();
> >     -        dup2(datafd[0], 0);
> >     -        reopen_fd_to_null(1);
> >     -        reopen_fd_to_null(2);
> >     -
> >     -        if (crypted) {
> >     -            execl(passwd_path, "chpasswd", "-e", NULL);
> >     -        } else {
> >     -            execl(passwd_path, "chpasswd", NULL);
> >     -        }
> >     -        _exit(EXIT_FAILURE);
> >     -    } else if (pid < 0) {
> >     -        error_setg_errno(errp, errno, "failed to create child
> >     process");
> >     -        goto out;
> >     -    }
> >     -    close(datafd[0]);
> >     -    datafd[0] = -1;
> >     -
> >     -    if (qemu_write_full(datafd[1], chpasswddata, chpasswdlen) !=
> >     chpasswdlen) {
> >     -        error_setg_errno(errp, errno, "cannot write new account
> >     password");
> >     -        goto out;
> >     -    }
> >     -    close(datafd[1]);
> >     -    datafd[1] = -1;
> >     -
> >     -    ga_wait_child(pid, &status, &local_err);
> >     -    if (local_err) {
> >     -        error_propagate(errp, local_err);
> >     -        goto out;
> >     -    }
> >     -
> >     -    if (!WIFEXITED(status)) {
> >     -        error_setg(errp, "child process has terminated abnormally");
> >     -        goto out;
> >     -    }
> >     -
> >     -    if (WEXITSTATUS(status)) {
> >     -        error_setg(errp, "child process has failed to set user
> >     password");
> >     -        goto out;
> >     -    }
> >     -
> >     -out:
> >     -    g_free(chpasswddata);
> >     -    g_free(rawpasswddata);
> >     -    g_free(passwd_path);
> >     -    if (datafd[0] != -1) {
> >     -        close(datafd[0]);
> >     -    }
> >     -    if (datafd[1] != -1) {
> >     -        close(datafd[1]);
> >     -    }
> >     -}
> >     -
> >      static void ga_read_sysfs_file(int dirfd, const char *pathname,
> >     char *buf,
> >                                     int size, Error **errp)
> >      {
> >     @@ -2793,6 +2688,124 @@ GuestMemoryBlockInfo
> >     *qmp_guest_get_memory_block_info(Error **errp)
> >
> >      #endif
> >
> >     +#if defined(__linux__) || defined(__FreeBSD__)
> >     +void qmp_guest_set_user_password(const char *username,
> >     +                                 const char *password,
> >     +                                 bool crypted,
> >     +                                 Error **errp)
> >     +{
> >     +    Error *local_err = NULL;
> >     +    char *passwd_path = NULL;
> >     +    pid_t pid;
> >     +    int status;
> >     +    int datafd[2] = { -1, -1 };
> >     +    char *rawpasswddata = NULL;
> >     +    size_t rawpasswdlen;
> >     +    char *chpasswddata = NULL;
> >     +    size_t chpasswdlen;
> >     +
> >     +    rawpasswddata = (char *)qbase64_decode(password, -1,
> >     &rawpasswdlen, errp);
> >     +    if (!rawpasswddata) {
> >     +        return;
> >     +    }
> >     +    rawpasswddata = g_renew(char, rawpasswddata, rawpasswdlen + 1);
> >     +    rawpasswddata[rawpasswdlen] = '\0';
> >     +
> >     +    if (strchr(rawpasswddata, '\n')) {
> >     +        error_setg(errp, "forbidden characters in raw password");
> >     +        goto out;
> >     +    }
> >     +
> >     +    if (strchr(username, '\n') ||
> >     +        strchr(username, ':')) {
> >     +        error_setg(errp, "forbidden characters in username");
> >     +        goto out;
> >     +    }
> >     +
> >     +#ifdef __FreeBSD__
> >     +    chpasswddata = g_strdup(rawpasswddata);
> >     +    passwd_path = g_find_program_in_path("pw");
> >     +#else
> >     +    chpasswddata = g_strdup_printf("%s:%s\n", username,
> >     rawpasswddata);
> >     +    passwd_path = g_find_program_in_path("chpasswd");
> >     +#endif
> >     +
> >     +    chpasswdlen = strlen(chpasswddata);
> >     +
> >     +    if (!passwd_path) {
> >     +        error_setg(errp, "cannot find 'passwd' program in PATH");
> >     +        goto out;
> >     +    }
> >     +
> >     +    if (!g_unix_open_pipe(datafd, FD_CLOEXEC, NULL)) {
> >     +        error_setg(errp, "cannot create pipe FDs");
> >     +        goto out;
> >     +    }
> >     +
> >     +    pid = fork();
> >     +    if (pid == 0) {
> >     +        close(datafd[1]);
> >     +        /* child */
> >     +        setsid();
> >     +        dup2(datafd[0], 0);
> >     +        reopen_fd_to_null(1);
> >     +        reopen_fd_to_null(2);
> >     +
> >     +#ifdef __FreeBSD__
> >     +        const char *h_arg;
> >     +        h_arg = (crypted) ? "-H" : "-h";
> >     +        execl(passwd_path, "pw", "usermod", "-n", username,
> >     h_arg, "0", NULL);
> >     +#else
> >     +        if (crypted) {
> >     +            execl(passwd_path, "chpasswd", "-e", NULL);
> >     +        } else {
> >     +            execl(passwd_path, "chpasswd", NULL);
> >     +        }
> >     +#endif
> >     +        _exit(EXIT_FAILURE);
> >     +    } else if (pid < 0) {
> >     +        error_setg_errno(errp, errno, "failed to create child
> >     process");
> >     +        goto out;
> >     +    }
> >     +    close(datafd[0]);
> >     +    datafd[0] = -1;
> >     +
> >     +    if (qemu_write_full(datafd[1], chpasswddata, chpasswdlen) !=
> >     chpasswdlen) {
> >     +        error_setg_errno(errp, errno, "cannot write new account
> >     password");
> >     +        goto out;
> >     +    }
> >     +    close(datafd[1]);
> >     +    datafd[1] = -1;
> >     +
> >     +    ga_wait_child(pid, &status, &local_err);
> >     +    if (local_err) {
> >     +        error_propagate(errp, local_err);
> >     +        goto out;
> >     +    }
> >     +
> >     +    if (!WIFEXITED(status)) {
> >     +        error_setg(errp, "child process has terminated abnormally");
> >     +        goto out;
> >     +    }
> >     +
> >     +    if (WEXITSTATUS(status)) {
> >     +        error_setg(errp, "child process has failed to set user
> >     password");
> >     +        goto out;
> >     +    }
> >     +
> >     +out:
> >     +    g_free(chpasswddata);
> >     +    g_free(rawpasswddata);
> >     +    g_free(passwd_path);
> >     +    if (datafd[0] != -1) {
> >     +        close(datafd[0]);
> >     +    }
> >     +    if (datafd[1] != -1) {
> >     +        close(datafd[1]);
> >     +    }
> >     +}
> >     +#endif
> >     +
> >      #ifdef HAVE_GETIFADDRS
> >      static GuestNetworkInterface *
> >      guest_find_interface(GuestNetworkInterfaceList *head,
> >     --
> >     2.34.1
> >
> >
> >
> >
> > --
> > Marc-André Lureau
>


-- 
Marc-André Lureau

[-- Attachment #2: Type: text/html, Size: 16027 bytes --]

^ permalink raw reply related	[flat|nested] 20+ messages in thread

* Re: [PATCH 5/7] qga: Add support for user password setting in FreeBSD
  2022-09-30  8:19       ` Marc-André Lureau
@ 2022-09-30 13:11         ` Alexander Ivanov
  0 siblings, 0 replies; 20+ messages in thread
From: Alexander Ivanov @ 2022-09-30 13:11 UTC (permalink / raw)
  To: Marc-André Lureau; +Cc: qemu-devel, den, michael.roth, kkostiuk

Hi

On 30.09.2022 10:19, Marc-André Lureau wrote:
>
> 	
> Caution: This is an external email and has a suspicious subject or 
> content. Please take care when clicking links or opening attachments. 
> When in doubt, contact your IT Department
>
>
> Hi
>
> On Thu, Sep 29, 2022 at 6:29 PM Alexander Ivanov 
> <alexander.ivanov@virtuozzo.com> wrote:
>
>
>     On 29.09.2022 13:22, Marc-André Lureau wrote:
>     >
>     >
>     > Caution: This is an external email and has a suspicious subject or
>     > content. Please take care when clicking links or opening
>     attachments.
>     > When in doubt, contact your IT Department
>     >
>     >
>     > Hi Alexander
>     >
>     > On Thu, Sep 29, 2022 at 12:52 PM Alexander Ivanov
>     > <alexander.ivanov@virtuozzo.com> wrote:
>     >
>     >     Move qmp_guest_set_user_password() from __linux__ condition to
>     >     (__linux__ || __FreeBSD__) condition. Add command and arguments
>     >     for password setting in FreeBSD.
>     >
>     >     Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
>     >
>     >
>     > Could you explain why you need to move the code?
>     >
>     > You could instead have a top declaration?
>     Now qmp_guest_set_user_password()is under #ifdef __linux__. I want to
>     use this function for FreeBSD too, so I moved it under (__linux__ ||
>     __FreeBSD__) condition. I could create another function for
>     FreeBSD but
>
>
> Why not make the following change?
I thought it is a bad idea to multiply pieces of code under the same 
arch conditions. OK, I will change the code in this way. Thank you.
>
> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> index 88e0d0fe24..78a345c9f3 100644
> --- a/qga/commands-posix.c
> +++ b/qga/commands-posix.c
> @@ -2122,7 +2122,9 @@ int64_t 
> qmp_guest_set_vcpus(GuestLogicalProcessorList *vcpus, Error **errp)
>
>      return processed;
>  }
> +#endif /* linux */
>
> +#if defined(__linux__) || defined(__FreeBSD__)
>  void qmp_guest_set_user_password(const char *username,
>                                   const char *password,
>                                   bool crypted,
> @@ -2227,7 +2229,9 @@ out:
>          close(datafd[1]);
>      }
>  }
> +#endif /* linux || freebsd */
>
> +#if defined(__linux__)
>  static void ga_read_sysfs_file(int dirfd, const char *pathname, char 
> *buf,
>                                 int size, Error **errp)
>  {
>
>     it would lead to code duplication. Unfortunately I don't see
>     another way
>     to do it except to add (__linux__ || __FreeBSD__) condition, cut the
>     function and paste in this condition. If you could suggest another
>     way I
>     would happy.
>     > If it's really required, please split the patch in move + additions.
>     Just moving the function without other changes leads to
>     unbuildable code
>     in FreeBSD. I thought it's worse than moving and changing at the same
>     time. I split the patch if you prefer.
>
>     >
>     >     ---
>     >      qga/commands-posix.c | 223
>     >     +++++++++++++++++++++++--------------------
>     >      1 file changed, 118 insertions(+), 105 deletions(-)
>     >
>     >     diff --git a/qga/commands-posix.c b/qga/commands-posix.c
>     >     index 88e0d0fe24..6ce894ca6e 100644
>     >     --- a/qga/commands-posix.c
>     >     +++ b/qga/commands-posix.c
>     >     @@ -2123,111 +2123,6 @@ int64_t
>     >     qmp_guest_set_vcpus(GuestLogicalProcessorList *vcpus, Error
>     **errp)
>     >          return processed;
>     >      }
>     >
>     >     -void qmp_guest_set_user_password(const char *username,
>     >     -                                 const char *password,
>     >     -                                 bool crypted,
>     >     -                                 Error **errp)
>     >     -{
>     >     -    Error *local_err = NULL;
>     >     -    char *passwd_path = NULL;
>     >     -    pid_t pid;
>     >     -    int status;
>     >     -    int datafd[2] = { -1, -1 };
>     >     -    char *rawpasswddata = NULL;
>     >     -    size_t rawpasswdlen;
>     >     -    char *chpasswddata = NULL;
>     >     -    size_t chpasswdlen;
>     >     -
>     >     -    rawpasswddata = (char *)qbase64_decode(password, -1,
>     >     &rawpasswdlen, errp);
>     >     -    if (!rawpasswddata) {
>     >     -        return;
>     >     -    }
>     >     -    rawpasswddata = g_renew(char, rawpasswddata,
>     rawpasswdlen + 1);
>     >     -    rawpasswddata[rawpasswdlen] = '\0';
>     >     -
>     >     -    if (strchr(rawpasswddata, '\n')) {
>     >     -        error_setg(errp, "forbidden characters in raw
>     password");
>     >     -        goto out;
>     >     -    }
>     >     -
>     >     -    if (strchr(username, '\n') ||
>     >     -        strchr(username, ':')) {
>     >     -        error_setg(errp, "forbidden characters in username");
>     >     -        goto out;
>     >     -    }
>     >     -
>     >     -    chpasswddata = g_strdup_printf("%s:%s\n", username,
>     >     rawpasswddata);
>     >     -    chpasswdlen = strlen(chpasswddata);
>     >     -
>     >     -    passwd_path = g_find_program_in_path("chpasswd");
>     >     -
>     >     -    if (!passwd_path) {
>     >     -        error_setg(errp, "cannot find 'passwd' program in
>     PATH");
>     >     -        goto out;
>     >     -    }
>     >     -
>     >     -    if (!g_unix_open_pipe(datafd, FD_CLOEXEC, NULL)) {
>     >     -        error_setg(errp, "cannot create pipe FDs");
>     >     -        goto out;
>     >     -    }
>     >     -
>     >     -    pid = fork();
>     >     -    if (pid == 0) {
>     >     -        close(datafd[1]);
>     >     -        /* child */
>     >     -        setsid();
>     >     -        dup2(datafd[0], 0);
>     >     -        reopen_fd_to_null(1);
>     >     -        reopen_fd_to_null(2);
>     >     -
>     >     -        if (crypted) {
>     >     -            execl(passwd_path, "chpasswd", "-e", NULL);
>     >     -        } else {
>     >     -            execl(passwd_path, "chpasswd", NULL);
>     >     -        }
>     >     -        _exit(EXIT_FAILURE);
>     >     -    } else if (pid < 0) {
>     >     -        error_setg_errno(errp, errno, "failed to create child
>     >     process");
>     >     -        goto out;
>     >     -    }
>     >     -    close(datafd[0]);
>     >     -    datafd[0] = -1;
>     >     -
>     >     -    if (qemu_write_full(datafd[1], chpasswddata,
>     chpasswdlen) !=
>     >     chpasswdlen) {
>     >     -        error_setg_errno(errp, errno, "cannot write new account
>     >     password");
>     >     -        goto out;
>     >     -    }
>     >     -    close(datafd[1]);
>     >     -    datafd[1] = -1;
>     >     -
>     >     -    ga_wait_child(pid, &status, &local_err);
>     >     -    if (local_err) {
>     >     -        error_propagate(errp, local_err);
>     >     -        goto out;
>     >     -    }
>     >     -
>     >     -    if (!WIFEXITED(status)) {
>     >     -        error_setg(errp, "child process has terminated
>     abnormally");
>     >     -        goto out;
>     >     -    }
>     >     -
>     >     -    if (WEXITSTATUS(status)) {
>     >     -        error_setg(errp, "child process has failed to set user
>     >     password");
>     >     -        goto out;
>     >     -    }
>     >     -
>     >     -out:
>     >     -    g_free(chpasswddata);
>     >     -    g_free(rawpasswddata);
>     >     -    g_free(passwd_path);
>     >     -    if (datafd[0] != -1) {
>     >     -        close(datafd[0]);
>     >     -    }
>     >     -    if (datafd[1] != -1) {
>     >     -        close(datafd[1]);
>     >     -    }
>     >     -}
>     >     -
>     >      static void ga_read_sysfs_file(int dirfd, const char *pathname,
>     >     char *buf,
>     >                                     int size, Error **errp)
>     >      {
>     >     @@ -2793,6 +2688,124 @@ GuestMemoryBlockInfo
>     >     *qmp_guest_get_memory_block_info(Error **errp)
>     >
>     >      #endif
>     >
>     >     +#if defined(__linux__) || defined(__FreeBSD__)
>     >     +void qmp_guest_set_user_password(const char *username,
>     >     +                                 const char *password,
>     >     +                                 bool crypted,
>     >     +                                 Error **errp)
>     >     +{
>     >     +    Error *local_err = NULL;
>     >     +    char *passwd_path = NULL;
>     >     +    pid_t pid;
>     >     +    int status;
>     >     +    int datafd[2] = { -1, -1 };
>     >     +    char *rawpasswddata = NULL;
>     >     +    size_t rawpasswdlen;
>     >     +    char *chpasswddata = NULL;
>     >     +    size_t chpasswdlen;
>     >     +
>     >     +    rawpasswddata = (char *)qbase64_decode(password, -1,
>     >     &rawpasswdlen, errp);
>     >     +    if (!rawpasswddata) {
>     >     +        return;
>     >     +    }
>     >     +    rawpasswddata = g_renew(char, rawpasswddata,
>     rawpasswdlen + 1);
>     >     +    rawpasswddata[rawpasswdlen] = '\0';
>     >     +
>     >     +    if (strchr(rawpasswddata, '\n')) {
>     >     +        error_setg(errp, "forbidden characters in raw
>     password");
>     >     +        goto out;
>     >     +    }
>     >     +
>     >     +    if (strchr(username, '\n') ||
>     >     +        strchr(username, ':')) {
>     >     +        error_setg(errp, "forbidden characters in username");
>     >     +        goto out;
>     >     +    }
>     >     +
>     >     +#ifdef __FreeBSD__
>     >     +    chpasswddata = g_strdup(rawpasswddata);
>     >     +    passwd_path = g_find_program_in_path("pw");
>     >     +#else
>     >     +    chpasswddata = g_strdup_printf("%s:%s\n", username,
>     >     rawpasswddata);
>     >     +    passwd_path = g_find_program_in_path("chpasswd");
>     >     +#endif
>     >     +
>     >     +    chpasswdlen = strlen(chpasswddata);
>     >     +
>     >     +    if (!passwd_path) {
>     >     +        error_setg(errp, "cannot find 'passwd' program in
>     PATH");
>     >     +        goto out;
>     >     +    }
>     >     +
>     >     +    if (!g_unix_open_pipe(datafd, FD_CLOEXEC, NULL)) {
>     >     +        error_setg(errp, "cannot create pipe FDs");
>     >     +        goto out;
>     >     +    }
>     >     +
>     >     +    pid = fork();
>     >     +    if (pid == 0) {
>     >     +        close(datafd[1]);
>     >     +        /* child */
>     >     +        setsid();
>     >     +        dup2(datafd[0], 0);
>     >     +        reopen_fd_to_null(1);
>     >     +        reopen_fd_to_null(2);
>     >     +
>     >     +#ifdef __FreeBSD__
>     >     +        const char *h_arg;
>     >     +        h_arg = (crypted) ? "-H" : "-h";
>     >     +        execl(passwd_path, "pw", "usermod", "-n", username,
>     >     h_arg, "0", NULL);
>     >     +#else
>     >     +        if (crypted) {
>     >     +            execl(passwd_path, "chpasswd", "-e", NULL);
>     >     +        } else {
>     >     +            execl(passwd_path, "chpasswd", NULL);
>     >     +        }
>     >     +#endif
>     >     +        _exit(EXIT_FAILURE);
>     >     +    } else if (pid < 0) {
>     >     +        error_setg_errno(errp, errno, "failed to create child
>     >     process");
>     >     +        goto out;
>     >     +    }
>     >     +    close(datafd[0]);
>     >     +    datafd[0] = -1;
>     >     +
>     >     +    if (qemu_write_full(datafd[1], chpasswddata,
>     chpasswdlen) !=
>     >     chpasswdlen) {
>     >     +        error_setg_errno(errp, errno, "cannot write new account
>     >     password");
>     >     +        goto out;
>     >     +    }
>     >     +    close(datafd[1]);
>     >     +    datafd[1] = -1;
>     >     +
>     >     +    ga_wait_child(pid, &status, &local_err);
>     >     +    if (local_err) {
>     >     +        error_propagate(errp, local_err);
>     >     +        goto out;
>     >     +    }
>     >     +
>     >     +    if (!WIFEXITED(status)) {
>     >     +        error_setg(errp, "child process has terminated
>     abnormally");
>     >     +        goto out;
>     >     +    }
>     >     +
>     >     +    if (WEXITSTATUS(status)) {
>     >     +        error_setg(errp, "child process has failed to set user
>     >     password");
>     >     +        goto out;
>     >     +    }
>     >     +
>     >     +out:
>     >     +    g_free(chpasswddata);
>     >     +    g_free(rawpasswddata);
>     >     +    g_free(passwd_path);
>     >     +    if (datafd[0] != -1) {
>     >     +        close(datafd[0]);
>     >     +    }
>     >     +    if (datafd[1] != -1) {
>     >     +        close(datafd[1]);
>     >     +    }
>     >     +}
>     >     +#endif
>     >     +
>     >      #ifdef HAVE_GETIFADDRS
>     >      static GuestNetworkInterface *
>     >      guest_find_interface(GuestNetworkInterfaceList *head,
>     >     --
>     >     2.34.1
>     >
>     >
>     >
>     >
>     > --
>     > Marc-André Lureau
>
>
>
> -- 
> Marc-André Lureau


^ permalink raw reply	[flat|nested] 20+ messages in thread

end of thread, other threads:[~2022-09-30 13:22 UTC | newest]

Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-09-29  7:52 [PATCH v2 0/7] qga: Add FreeBSD support Alexander Ivanov
2022-09-29  7:52 ` [PATCH 1/7] qga: Add initial " Alexander Ivanov
2022-09-29 11:29   ` Marc-André Lureau
2022-09-29 14:11     ` Alexander Ivanov
2022-09-29  7:52 ` [PATCH 2/7] qga: Move Linux-specific FS freeze/thaw code to a separate file Alexander Ivanov
2022-09-29 11:29   ` Marc-André Lureau
2022-09-29  7:52 ` [PATCH 3/7] qga: Add UFS freeze/thaw support for FreeBSD Alexander Ivanov
2022-09-29 11:28   ` Marc-André Lureau
2022-09-29  7:52 ` [PATCH 4/7] qga: Add shutdown/halt/reboot " Alexander Ivanov
2022-09-29 11:28   ` Marc-André Lureau
2022-09-29  7:52 ` [PATCH 5/7] qga: Add support for user password setting in FreeBSD Alexander Ivanov
2022-09-29 11:22   ` Marc-André Lureau
2022-09-29 14:29     ` Alexander Ivanov
2022-09-30  8:19       ` Marc-André Lureau
2022-09-30 13:11         ` Alexander Ivanov
2022-09-29  7:52 ` [PATCH 6/7] qga: Move HW address getting to a separate function Alexander Ivanov
2022-09-29 11:28   ` Marc-André Lureau
2022-09-29 14:33     ` Alexander Ivanov
2022-09-29  7:52 ` [PATCH 7/7] qga: Add HW address getting for FreeBSD Alexander Ivanov
2022-09-29 11:28   ` Marc-André Lureau

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.