Signed-off-by: Rafael Fonseca <r4f4rfs@gmail.com>
---
src/libxl/libxl_migration.c | 65 +++++++++++++++++++++----------------
1 file changed, 37 insertions(+), 28 deletions(-)
diff --git a/src/libxl/libxl_migration.c b/src/libxl/libxl_migration.c
index 50225855ae..678d850cf6 100644
--- a/src/libxl/libxl_migration.c
+++ b/src/libxl/libxl_migration.c
@@ -60,7 +60,7 @@ struct _libxlMigrationCookie {
};
typedef struct _libxlMigrationDstArgs {
- virObject parent;
+ GObject parent;
int recvfd;
virConnectPtr conn;
@@ -73,7 +73,28 @@ typedef struct _libxlMigrationDstArgs {
size_t nsocks;
} libxlMigrationDstArgs;
-static virClassPtr libxlMigrationDstArgsClass;
+G_DEFINE_TYPE(libxlMigrationDstArgs, libxl_migration_dst_args, G_TYPE_OBJECT);
+#define LIBXL_TYPE_MIGRATION_DST_ARGS libxl_migration_dst_args_get_type()
+G_DECLARE_FINAL_TYPE(libxlMigrationDstArgs,
+ libxl_migration_dst_args,
+ LIBXL,
+ MIGRATION_DST_ARGS,
+ GObject);
+
+static void libxlMigrationDstArgsFinalize(GObject *obj);
+
+static void
+libxl_migration_dst_args_init(lixlMigrationDstArgs *args G_GNUC_UNUSED)
+{
+}
+
+static void
+libxl_migration_dst_args_class_init(lixlMigrationDstArgsClass *klass)
+{
+ GObjectClass *obj = G_OBJECT_CLASS(klass);
+
+ obj->finalize = libxlMigrationDstArgsFinalize;
+}
static void
@@ -226,31 +247,23 @@ libxlMigrationEatCookie(const char *cookiein,
}
static void
-libxlMigrationDstArgsDispose(void *obj)
+libxlMigrationDstArgsFinalize(GObject *obj)
{
- libxlMigrationDstArgs *args = obj;
+ libxlMigrationDstArgs *args = LIBXL_MIGRATION_DST_ARGS(obj);
libxlMigrationCookieFree(args->migcookie);
VIR_FREE(args->socks);
virObjectUnref(args->conn);
virObjectUnref(args->vm);
-}
-static int
-libxlMigrationDstArgsOnceInit(void)
-{
- if (!VIR_CLASS_NEW(libxlMigrationDstArgs, virClassForObject()))
- return -1;
-
- return 0;
+ G_OBJECT_CLASS(libxl_migration_dst_args_parent_class)->finalize(obj);
}
-VIR_ONCE_GLOBAL_INIT(libxlMigrationDstArgs);
static void
libxlDoMigrateDstReceive(void *opaque)
{
- libxlMigrationDstArgs *args = opaque;
+ g_autoptr(libxlMigrationDstArgs) args = LIBXL_MIGRATION_DST_ARGS(opaque);
virDomainObjPtr vm = args->vm;
virNetSocketPtr *socks = args->socks;
size_t nsocks = args->nsocks;
@@ -277,7 +290,6 @@ libxlDoMigrateDstReceive(void *opaque)
}
args->nsocks = 0;
VIR_FORCE_CLOSE(recvfd);
- virObjectUnref(args);
virDomainObjEndAPI(&vm);
}
@@ -287,7 +299,7 @@ libxlMigrateDstReceive(virNetSocketPtr sock,
int events G_GNUC_UNUSED,
void *opaque)
{
- libxlMigrationDstArgs *args = opaque;
+ g_autoptr(libxlMigrationDstArgs) args = LIBXL_MIGRATION_DST_ARGS(opaque);
virNetSocketPtr *socks = args->socks;
size_t nsocks = args->nsocks;
libxlDomainObjPrivatePtr priv = args->vm->privateData;
@@ -321,7 +333,7 @@ libxlMigrateDstReceive(virNetSocketPtr sock,
libxlDoMigrateDstReceive,
name,
false,
- args) < 0) {
+ g_object_ref(args)) < 0) {
virReportError(VIR_ERR_OPERATION_FAILED, "%s",
_("Failed to create thread for receiving migration data"));
goto fail;
@@ -339,7 +351,6 @@ libxlMigrateDstReceive(virNetSocketPtr sock,
}
args->nsocks = 0;
VIR_FORCE_CLOSE(recvfd);
- virObjectUnref(args);
}
static int
@@ -551,7 +562,7 @@ libxlDomainMigrationDstPrepareTunnel3(virConnectPtr dconn,
libxlMigrationCookiePtr mig = NULL;
libxlDriverPrivatePtr driver = dconn->privateData;
virDomainObjPtr vm = NULL;
- libxlMigrationDstArgs *args = NULL;
+ g_autoptr(libxlMigrationDstArgs) args = NULL;
bool taint_hook = false;
libxlDomainObjPrivatePtr priv = NULL;
char *xmlout = NULL;
@@ -600,8 +611,8 @@ libxlDomainMigrationDstPrepareTunnel3(virConnectPtr dconn,
if (libxlMigrationDstArgsInitialize() < 0)
goto endjob;
- if (!(args = virObjectNew(libxlMigrationDstArgsClass)))
- goto endjob;
+ args = LIBXL_MIGRATION_DST_ARGS(
+ g_object_new(LIBXL_TYPE_MIGRATION_DST_ARGS, NULL));
args->conn = virObjectRef(dconn);
args->vm = virObjectRef(vm);
@@ -618,7 +629,7 @@ libxlDomainMigrationDstPrepareTunnel3(virConnectPtr dconn,
name = g_strdup_printf("mig-%s", args->vm->def->name);
if (virThreadCreateFull(priv->migrationDstReceiveThr, true,
libxlDoMigrateDstReceive,
- name, false, args) < 0) {
+ name, false, g_object_ref(args)) < 0) {
virReportError(VIR_ERR_OPERATION_FAILED, "%s",
_("Failed to create thread for receiving migration data"));
goto endjob;
@@ -634,7 +645,6 @@ libxlDomainMigrationDstPrepareTunnel3(virConnectPtr dconn,
libxlMigrationCookieFree(mig);
VIR_FORCE_CLOSE(dataFD[1]);
VIR_FORCE_CLOSE(dataFD[0]);
- virObjectUnref(args);
/* Remove virDomainObj from domain list */
if (vm)
virDomainObjListRemove(driver->domains, vm);
@@ -665,7 +675,7 @@ libxlDomainMigrationDstPrepare(virConnectPtr dconn,
virNetSocketPtr *socks = NULL;
size_t nsocks = 0;
int nsocks_listen = 0;
- libxlMigrationDstArgs *args = NULL;
+ g_autoptr(libxlMigrationDstArgs) args = NULL;
bool taint_hook = false;
libxlDomainObjPrivatePtr priv = NULL;
size_t i;
@@ -765,8 +775,8 @@ libxlDomainMigrationDstPrepare(virConnectPtr dconn,
if (libxlMigrationDstArgsInitialize() < 0)
goto endjob;
- if (!(args = virObjectNew(libxlMigrationDstArgsClass)))
- goto endjob;
+ args = LIBXL_MIGRATION_DST_ARGS(
+ g_object_new(LIBXL_TYPE_MIGRATION_DST_ARGS, NULL));
args->conn = virObjectRef(dconn);
args->vm = virObjectRef(vm);
@@ -786,7 +796,7 @@ libxlDomainMigrationDstPrepare(virConnectPtr dconn,
if (virNetSocketAddIOCallback(socks[i],
VIR_EVENT_HANDLE_READABLE,
libxlMigrateDstReceive,
- virObjectRef(args),
+ g_object_ref(args),
NULL) < 0)
continue;
@@ -823,7 +833,6 @@ libxlDomainMigrationDstPrepare(virConnectPtr dconn,
VIR_FREE(hostname);
else
virURIFree(uri);
- virObjectUnref(args);
virDomainObjEndAPI(&vm);
return ret;
}
--
2.26.2
On Wed, May 13, 2020 at 01:57:19PM +0200, Rafael Fonseca wrote:
> Signed-off-by: Rafael Fonseca <r4f4rfs@gmail.com>
> ---
> src/libxl/libxl_migration.c | 65 +++++++++++++++++++++----------------
> 1 file changed, 37 insertions(+), 28 deletions(-)
>
> diff --git a/src/libxl/libxl_migration.c b/src/libxl/libxl_migration.c
> index 50225855ae..678d850cf6 100644
> --- a/src/libxl/libxl_migration.c
> +++ b/src/libxl/libxl_migration.c
This doesn't seem to have even been compile tested.
Since we're using GitLab now, if you just fork the libvirt repo
and push a branch to your gitlab fork, it will run CI which will
check for these mistakes.
diff --git a/src/libxl/libxl_migration.c b/src/libxl/libxl_migration.c
index 678d850cf6..3e07f477c6 100644
--- a/src/libxl/libxl_migration.c
+++ b/src/libxl/libxl_migration.c
@@ -73,23 +73,23 @@ typedef struct _libxlMigrationDstArgs {
size_t nsocks;
} libxlMigrationDstArgs;
-G_DEFINE_TYPE(libxlMigrationDstArgs, libxl_migration_dst_args, G_TYPE_OBJECT);
#define LIBXL_TYPE_MIGRATION_DST_ARGS libxl_migration_dst_args_get_type()
G_DECLARE_FINAL_TYPE(libxlMigrationDstArgs,
libxl_migration_dst_args,
LIBXL,
MIGRATION_DST_ARGS,
GObject);
+G_DEFINE_TYPE(libxlMigrationDstArgs, libxl_migration_dst_args, G_TYPE_OBJECT);
static void libxlMigrationDstArgsFinalize(GObject *obj);
static void
-libxl_migration_dst_args_init(lixlMigrationDstArgs *args G_GNUC_UNUSED)
+libxl_migration_dst_args_init(libxlMigrationDstArgs *args G_GNUC_UNUSED)
{
}
static void
-libxl_migration_dst_args_class_init(lixlMigrationDstArgsClass *klass)
+libxl_migration_dst_args_class_init(libxlMigrationDstArgsClass *klass)
{
GObjectClass *obj = G_OBJECT_CLASS(klass);
@@ -608,9 +608,6 @@ libxlDomainMigrationDstPrepareTunnel3(virConnectPtr dconn,
goto endjob;
dataFD[1] = -1; /* 'st' owns the FD now & will close it */
- if (libxlMigrationDstArgsInitialize() < 0)
- goto endjob;
-
args = LIBXL_MIGRATION_DST_ARGS(
g_object_new(LIBXL_TYPE_MIGRATION_DST_ARGS, NULL));
@@ -772,9 +769,6 @@ libxlDomainMigrationDstPrepare(virConnectPtr dconn,
goto endjob;
}
- if (libxlMigrationDstArgsInitialize() < 0)
- goto endjob;
-
args = LIBXL_MIGRATION_DST_ARGS(
g_object_new(LIBXL_TYPE_MIGRATION_DST_ARGS, NULL));
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Fri, May 15, 2020 at 5:24 PM Daniel P. Berrangé <berrange@redhat.com> wrote: > Since we're using GitLab now, if you just fork the libvirt repo > and push a branch to your gitlab fork, it will run CI which will > check for these mistakes. Good to know. I did that and the CI is only failing now in the cross-compilation phase with 32-bit systems: ../../src/util/virstoragefile.h: In function 'VIR_STORAGE_SOURCE': /usr/i686-w64-mingw32/sys-root/mingw/include/glib-2.0/gobject/gtype.h:2301:6: error: cast increases required alignment of target type [-Werror=cast-align] 2301 | ((ct*) g_type_check_instance_cast ((GTypeInstance*) ip, gt)) | ^ /usr/i686-w64-mingw32/sys-root/mingw/include/glib-2.0/gobject/gtype.h:484:66: note: in expansion of macro '_G_TYPE_CIC' 484 | #define G_TYPE_CHECK_INSTANCE_CAST(instance, g_type, c_type) (_G_TYPE_CIC ((instance), (g_type), c_type)) | ^~~~~~~~~~~ /usr/i686-w64-mingw32/sys-root/mingw/include/glib-2.0/gobject/gtype.h:1412:12: note: in expansion of macro 'G_TYPE_CHECK_INSTANCE_CAST' 1412 | return G_TYPE_CHECK_INSTANCE_CAST (ptr, module_obj_name##_get_type (), ModuleObjName); } \ | ^~~~~~~~~~~~~~~~~~~~~~~~~~ ../../src/util/virstoragefile.h:390:1: note: in expansion of macro 'G_DECLARE_FINAL_TYPE' 390 | G_DECLARE_FINAL_TYPE(virStorageSource, vir_storage_source, VIR, STORAGE_SOURCE, GObject); | ^~~~~~~~~~~~~~~~~~~~ Any ideas on how to solve that? Att -- Rafael Fonseca
On Sun, May 17, 2020 at 11:03:57PM +0200, Rafael Fonseca wrote: > On Fri, May 15, 2020 at 5:24 PM Daniel P. Berrangé <berrange@redhat.com> wrote: > > Since we're using GitLab now, if you just fork the libvirt repo > > and push a branch to your gitlab fork, it will run CI which will > > check for these mistakes. > > Good to know. I did that and the CI is only failing now in the > cross-compilation phase with 32-bit systems: > > ../../src/util/virstoragefile.h: In function 'VIR_STORAGE_SOURCE': > /usr/i686-w64-mingw32/sys-root/mingw/include/glib-2.0/gobject/gtype.h:2301:6: > error: cast increases required alignment of target type > [-Werror=cast-align] > 2301 | ((ct*) g_type_check_instance_cast ((GTypeInstance*) ip, gt)) > | ^ > /usr/i686-w64-mingw32/sys-root/mingw/include/glib-2.0/gobject/gtype.h:484:66: > note: in expansion of macro '_G_TYPE_CIC' > 484 | #define G_TYPE_CHECK_INSTANCE_CAST(instance, g_type, c_type) > (_G_TYPE_CIC ((instance), (g_type), c_type)) > | ^~~~~~~~~~~ > /usr/i686-w64-mingw32/sys-root/mingw/include/glib-2.0/gobject/gtype.h:1412:12: > note: in expansion of macro 'G_TYPE_CHECK_INSTANCE_CAST' > 1412 | return G_TYPE_CHECK_INSTANCE_CAST (ptr, > module_obj_name##_get_type (), ModuleObjName); } \ > | ^~~~~~~~~~~~~~~~~~~~~~~~~~ > ../../src/util/virstoragefile.h:390:1: note: in expansion of macro > 'G_DECLARE_FINAL_TYPE' > 390 | G_DECLARE_FINAL_TYPE(virStorageSource, vir_storage_source, VIR, > STORAGE_SOURCE, GObject); > | ^~~~~~~~~~~~~~~~~~~~ > > Any ideas on how to solve that? This is odd and I can't tell why. Assyuming only virStorageSource shows the issue though, there must be something in the way the struct is arranged that causes alignment issues. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Mon, May 18, 2020 at 11:31 AM Daniel P. Berrangé <berrange@redhat.com> wrote: > > On Sun, May 17, 2020 at 11:03:57PM +0200, Rafael Fonseca wrote: > > On Fri, May 15, 2020 at 5:24 PM Daniel P. Berrangé <berrange@redhat.com> wrote: > > > Since we're using GitLab now, if you just fork the libvirt repo > > > and push a branch to your gitlab fork, it will run CI which will > > > check for these mistakes. > > > > Good to know. I did that and the CI is only failing now in the > > cross-compilation phase with 32-bit systems: > > > > ../../src/util/virstoragefile.h: In function 'VIR_STORAGE_SOURCE': > > /usr/i686-w64-mingw32/sys-root/mingw/include/glib-2.0/gobject/gtype.h:2301:6: > > error: cast increases required alignment of target type > > [-Werror=cast-align] > > 2301 | ((ct*) g_type_check_instance_cast ((GTypeInstance*) ip, gt)) > > | ^ > > /usr/i686-w64-mingw32/sys-root/mingw/include/glib-2.0/gobject/gtype.h:484:66: > > note: in expansion of macro '_G_TYPE_CIC' > > 484 | #define G_TYPE_CHECK_INSTANCE_CAST(instance, g_type, c_type) > > (_G_TYPE_CIC ((instance), (g_type), c_type)) > > | ^~~~~~~~~~~ > > /usr/i686-w64-mingw32/sys-root/mingw/include/glib-2.0/gobject/gtype.h:1412:12: > > note: in expansion of macro 'G_TYPE_CHECK_INSTANCE_CAST' > > 1412 | return G_TYPE_CHECK_INSTANCE_CAST (ptr, > > module_obj_name##_get_type (), ModuleObjName); } \ > > | ^~~~~~~~~~~~~~~~~~~~~~~~~~ > > ../../src/util/virstoragefile.h:390:1: note: in expansion of macro > > 'G_DECLARE_FINAL_TYPE' > > 390 | G_DECLARE_FINAL_TYPE(virStorageSource, vir_storage_source, VIR, > > STORAGE_SOURCE, GObject); > > | ^~~~~~~~~~~~~~~~~~~~ > > > > Any ideas on how to solve that? > > This is odd and I can't tell why. Assyuming only virStorageSource shows > the issue though, there must be something in the way the struct is > arranged that causes alignment issues. I disabled the virStorageSource patch and tried again. On armv7l it failed with the same error but in a different part: ../../src/qemu/qemu_conf.h: In function 'VIR_QEMU_DRIVER_CONFIG': /usr/include/glib-2.0/gobject/gtype.h:2301:6: error: cast increases required alignment of target type [-Werror=cast-align] 2301 | ((ct*) g_type_check_instance_cast ((GTypeInstance*) ip, gt)) | ^ /usr/include/glib-2.0/gobject/gtype.h:484:66: note: in expansion of macro '_G_TYPE_CIC' 484 | #define G_TYPE_CHECK_INSTANCE_CAST(instance, g_type, c_type) (_G_TYPE_CIC ((instance), (g_type), c_type)) | ^~~~~~~~~~~ /usr/include/glib-2.0/gobject/gtype.h:1412:12: note: in expansion of macro 'G_TYPE_CHECK_INSTANCE_CAST' 1412 | return G_TYPE_CHECK_INSTANCE_CAST (ptr, module_obj_name##_get_type (), ModuleObjName); } \ | ^~~~~~~~~~~~~~~~~~~~~~~~~~ ../../src/qemu/qemu_conf.h:222:1: note: in expansion of macro 'G_DECLARE_FINAL_TYPE' 222 | G_DECLARE_FINAL_TYPE(virQEMUDriverConfig, | ^~~~~~~~~~~~~~~~~~~~ It didn't occur in mingw32-fedora-rawhide because I guess the QEmu driver it's not compiled there? Att. -- Rafael Fonseca
On Mon, May 18, 2020 at 3:28 PM Rafael Fonseca <r4f4rfs@gmail.com> wrote: > > On Mon, May 18, 2020 at 11:31 AM Daniel P. Berrangé <berrange@redhat.com> wrote: > > > > On Sun, May 17, 2020 at 11:03:57PM +0200, Rafael Fonseca wrote: > > > On Fri, May 15, 2020 at 5:24 PM Daniel P. Berrangé <berrange@redhat.com> wrote: > > > > Since we're using GitLab now, if you just fork the libvirt repo > > > > and push a branch to your gitlab fork, it will run CI which will > > > > check for these mistakes. > > > > > > Good to know. I did that and the CI is only failing now in the > > > cross-compilation phase with 32-bit systems: > > > > > > ../../src/util/virstoragefile.h: In function 'VIR_STORAGE_SOURCE': > > > /usr/i686-w64-mingw32/sys-root/mingw/include/glib-2.0/gobject/gtype.h:2301:6: > > > error: cast increases required alignment of target type > > > [-Werror=cast-align] > > > 2301 | ((ct*) g_type_check_instance_cast ((GTypeInstance*) ip, gt)) > > > | ^ > > > /usr/i686-w64-mingw32/sys-root/mingw/include/glib-2.0/gobject/gtype.h:484:66: > > > note: in expansion of macro '_G_TYPE_CIC' > > > 484 | #define G_TYPE_CHECK_INSTANCE_CAST(instance, g_type, c_type) > > > (_G_TYPE_CIC ((instance), (g_type), c_type)) > > > | ^~~~~~~~~~~ > > > /usr/i686-w64-mingw32/sys-root/mingw/include/glib-2.0/gobject/gtype.h:1412:12: > > > note: in expansion of macro 'G_TYPE_CHECK_INSTANCE_CAST' > > > 1412 | return G_TYPE_CHECK_INSTANCE_CAST (ptr, > > > module_obj_name##_get_type (), ModuleObjName); } \ > > > | ^~~~~~~~~~~~~~~~~~~~~~~~~~ > > > ../../src/util/virstoragefile.h:390:1: note: in expansion of macro > > > 'G_DECLARE_FINAL_TYPE' > > > 390 | G_DECLARE_FINAL_TYPE(virStorageSource, vir_storage_source, VIR, > > > STORAGE_SOURCE, GObject); > > > | ^~~~~~~~~~~~~~~~~~~~ > > > > > > Any ideas on how to solve that? > > > > This is odd and I can't tell why. Assyuming only virStorageSource shows > > the issue though, there must be something in the way the struct is > > arranged that causes alignment issues. > > I disabled the virStorageSource patch and tried again. On armv7l it > failed with the same error but in a different part: > > ../../src/qemu/qemu_conf.h: In function 'VIR_QEMU_DRIVER_CONFIG': Just to complete the information, I also disabled the QEmuDriverConfig patch and the CI finished successfully in armv7l. So it's something in virStorageSource and QemuDriverConfig, either the structs themselves or something missing from my changes. Att. -- Rafael Fonseca
On Mon, May 18, 2020 at 05:16:29PM +0200, Rafael Fonseca wrote: > On Mon, May 18, 2020 at 3:28 PM Rafael Fonseca <r4f4rfs@gmail.com> wrote: > > > > On Mon, May 18, 2020 at 11:31 AM Daniel P. Berrangé <berrange@redhat.com> wrote: > > > > > > On Sun, May 17, 2020 at 11:03:57PM +0200, Rafael Fonseca wrote: > > > > On Fri, May 15, 2020 at 5:24 PM Daniel P. Berrangé <berrange@redhat.com> wrote: > > > > > Since we're using GitLab now, if you just fork the libvirt repo > > > > > and push a branch to your gitlab fork, it will run CI which will > > > > > check for these mistakes. > > > > > > > > Good to know. I did that and the CI is only failing now in the > > > > cross-compilation phase with 32-bit systems: > > > > > > > > ../../src/util/virstoragefile.h: In function 'VIR_STORAGE_SOURCE': > > > > /usr/i686-w64-mingw32/sys-root/mingw/include/glib-2.0/gobject/gtype.h:2301:6: > > > > error: cast increases required alignment of target type > > > > [-Werror=cast-align] > > > > 2301 | ((ct*) g_type_check_instance_cast ((GTypeInstance*) ip, gt)) > > > > | ^ > > > > /usr/i686-w64-mingw32/sys-root/mingw/include/glib-2.0/gobject/gtype.h:484:66: > > > > note: in expansion of macro '_G_TYPE_CIC' > > > > 484 | #define G_TYPE_CHECK_INSTANCE_CAST(instance, g_type, c_type) > > > > (_G_TYPE_CIC ((instance), (g_type), c_type)) > > > > | ^~~~~~~~~~~ > > > > /usr/i686-w64-mingw32/sys-root/mingw/include/glib-2.0/gobject/gtype.h:1412:12: > > > > note: in expansion of macro 'G_TYPE_CHECK_INSTANCE_CAST' > > > > 1412 | return G_TYPE_CHECK_INSTANCE_CAST (ptr, > > > > module_obj_name##_get_type (), ModuleObjName); } \ > > > > | ^~~~~~~~~~~~~~~~~~~~~~~~~~ > > > > ../../src/util/virstoragefile.h:390:1: note: in expansion of macro > > > > 'G_DECLARE_FINAL_TYPE' > > > > 390 | G_DECLARE_FINAL_TYPE(virStorageSource, vir_storage_source, VIR, > > > > STORAGE_SOURCE, GObject); > > > > | ^~~~~~~~~~~~~~~~~~~~ > > > > > > > > Any ideas on how to solve that? > > > > > > This is odd and I can't tell why. Assyuming only virStorageSource shows > > > the issue though, there must be something in the way the struct is > > > arranged that causes alignment issues. > > > > I disabled the virStorageSource patch and tried again. On armv7l it > > failed with the same error but in a different part: > > > > ../../src/qemu/qemu_conf.h: In function 'VIR_QEMU_DRIVER_CONFIG': > > Just to complete the information, I also disabled the QEmuDriverConfig > patch and the CI finished successfully in armv7l. So it's something in > virStorageSource and QemuDriverConfig, either the structs themselves > or something missing from my changes. You're not doing anything wrong. The compiler doesn't have enough info to realize that its warning is wrong. We have a "GObject *" which requires 4 byte alignment, and are trying to cast to "virStorageSource *" or "QemuDriverConfig *" which require 8 byte alignment. The compiler is pessimistic and has to consider that the original memory for the "GObject *" is only aligned to 4 byte boundary, and thus a bad cast would result if we cast to "virStorageSource *". What the compiler does not know is that any "GObject *" we have will always have been allocated using g_object_new, which delegates to g_slice_new, which uses malloc or a slab allocator. Both malloc & the glib slab allocator guarantee that all memory they allocate is aligned to 8 byte boundaries on 32-bit. So the only way the cast from GObject * -> virStorageSource * could be a bad cast is if we allocated the GObject on the stack but that'll never happen. So the cast alignment warnings are bogus. The difficulty is that I don't see a nice way to eliminate the warnings. Options I see include: 1 Turn off -Wcast-align entirely 2 Modify GLib to add attribute(align(8)) to GObject 3 Modify GLib to use Pragma push/pop to selectively disable -Wcast-align I think (2) is an ABI change is probably not possible. Possibly we have todo (1) and then also do (3) so we can revert (1) in future. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Thu, Jun 4, 2020 at 11:16 AM Daniel P. Berrangé <berrange@redhat.com> wrote: > > On Mon, May 18, 2020 at 05:16:29PM +0200, Rafael Fonseca wrote: > > On Mon, May 18, 2020 at 3:28 PM Rafael Fonseca <r4f4rfs@gmail.com> wrote: > > > > > > On Mon, May 18, 2020 at 11:31 AM Daniel P. Berrangé <berrange@redhat.com> wrote: > > > > > > > > On Sun, May 17, 2020 at 11:03:57PM +0200, Rafael Fonseca wrote: > > > > > On Fri, May 15, 2020 at 5:24 PM Daniel P. Berrangé <berrange@redhat.com> wrote: > > > > > > Since we're using GitLab now, if you just fork the libvirt repo > > > > > > and push a branch to your gitlab fork, it will run CI which will > > > > > > check for these mistakes. > > > > > > > > > > Good to know. I did that and the CI is only failing now in the > > > > > cross-compilation phase with 32-bit systems: > > > > > > > > > > ../../src/util/virstoragefile.h: In function 'VIR_STORAGE_SOURCE': > > > > > /usr/i686-w64-mingw32/sys-root/mingw/include/glib-2.0/gobject/gtype.h:2301:6: > > > > > error: cast increases required alignment of target type > > > > > [-Werror=cast-align] > > > > > 2301 | ((ct*) g_type_check_instance_cast ((GTypeInstance*) ip, gt)) > > > > > | ^ > > > > > /usr/i686-w64-mingw32/sys-root/mingw/include/glib-2.0/gobject/gtype.h:484:66: > > > > > note: in expansion of macro '_G_TYPE_CIC' > > > > > 484 | #define G_TYPE_CHECK_INSTANCE_CAST(instance, g_type, c_type) > > > > > (_G_TYPE_CIC ((instance), (g_type), c_type)) > > > > > | ^~~~~~~~~~~ > > > > > /usr/i686-w64-mingw32/sys-root/mingw/include/glib-2.0/gobject/gtype.h:1412:12: > > > > > note: in expansion of macro 'G_TYPE_CHECK_INSTANCE_CAST' > > > > > 1412 | return G_TYPE_CHECK_INSTANCE_CAST (ptr, > > > > > module_obj_name##_get_type (), ModuleObjName); } \ > > > > > | ^~~~~~~~~~~~~~~~~~~~~~~~~~ > > > > > ../../src/util/virstoragefile.h:390:1: note: in expansion of macro > > > > > 'G_DECLARE_FINAL_TYPE' > > > > > 390 | G_DECLARE_FINAL_TYPE(virStorageSource, vir_storage_source, VIR, > > > > > STORAGE_SOURCE, GObject); > > > > > | ^~~~~~~~~~~~~~~~~~~~ > > > > > > > > > > Any ideas on how to solve that? > > > > > > > > This is odd and I can't tell why. Assyuming only virStorageSource shows > > > > the issue though, there must be something in the way the struct is > > > > arranged that causes alignment issues. > > > > > > I disabled the virStorageSource patch and tried again. On armv7l it > > > failed with the same error but in a different part: > > > > > > ../../src/qemu/qemu_conf.h: In function 'VIR_QEMU_DRIVER_CONFIG': > > > > Just to complete the information, I also disabled the QEmuDriverConfig > > patch and the CI finished successfully in armv7l. So it's something in > > virStorageSource and QemuDriverConfig, either the structs themselves > > or something missing from my changes. > > You're not doing anything wrong. The compiler doesn't have enough info to > realize that its warning is wrong. > > We have a "GObject *" which requires 4 byte alignment, and are trying to > cast to "virStorageSource *" or "QemuDriverConfig *" which require 8 byte > alignment. > > The compiler is pessimistic and has to consider that the original memory > for the "GObject *" is only aligned to 4 byte boundary, and thus a bad > cast would result if we cast to "virStorageSource *". > > What the compiler does not know is that any "GObject *" we have will always > have been allocated using g_object_new, which delegates to g_slice_new, > which uses malloc or a slab allocator. Both malloc & the glib slab allocator > guarantee that all memory they allocate is aligned to 8 byte boundaries on > 32-bit. > > So the only way the cast from GObject * -> virStorageSource * could be a > bad cast is if we allocated the GObject on the stack but that'll never > happen. > > So the cast alignment warnings are bogus. > > The difficulty is that I don't see a nice way to eliminate the warnings. > Options I see include: > > 1 Turn off -Wcast-align entirely > 2 Modify GLib to add attribute(align(8)) to GObject > 3 Modify GLib to use Pragma push/pop to selectively disable -Wcast-align > > I think (2) is an ABI change is probably not possible. Possibly we have > todo (1) and then also do (3) so we can revert (1) in future. Thank you for the write up. I don't have much experience with alignment issues, so any help is appreciated. I was doing some experiments and ended up replacing the macro G_DECLARE_FINAL_TYPE by the actual code. The problem doesn't seem to be the cast virStorageSource* -> GObject* but virStorageSource* -> GTypeInstance*. GTypeInstance struct has a pointer to a GTypeClass which has a GType field. GType == gsize. This cast in GLib's code is guarded by the #ifndef G_DISABLE_CAST_CHECKS [https://gitlab.gnome.org/GNOME/glib/-/blob/master/gobject/gtype.h#L2295]. In case that's defined, it skips the g_type_check_instance_cast call and just does (virStorageSource*)obj. So I did that in the code and there were no compiler warnings for virStorageSource. Does this information help in any way? Is "unrolling" the macro by the actual code for these 2 structs (virStorageSource and virQEMUDriverConfig) and replacing the cast checking by just a pure cast an option (4)? Att. -- Rafael Fonseca
© 2016 - 2026 Red Hat, Inc.