* [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* 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
* [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* 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
* [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* 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
* [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* 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
* [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* 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 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 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
* [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* 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 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
* [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 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