[PATCH 0/8] Decrease stack size below 2048 bytes
Inspired by Roman's patch: https://lists.libvirt.org/archives/list/devel@lists.libvirt.org/thread/FNAFO... There are two functions where clang produces stack greater than 2048 bytes (our current limit - see @stack_frame_size in meson.build): 1) doRemoteOpen() 2) vboxSnapshotRedefine() It took me a while to realize why a function with a dozen variables requires ~2300 bytes of stack, until the usual suspect turned in - glib. Anyway, green pipeline: https://gitlab.com/MichalPrivoznik/libvirt/-/pipelines/1752604895 and I've verified this compiles cleanly on my FreeBSD-14.1-RELEASE-p4 VM. Michal Prívozník (8): remote_driver: Move URI arg extraction into a separate function doRemoteOpen: Move RPC talk to a separate function doRemoteOpen: Drop needless typecast of @transport vbox: Move parts of vboxSnapshotRedefine() into a separate function vbox: Move parts of vboxSnapshotRedefine() into a separate function vbox: Move parts of vboxSnapshotRedefine() into a separate function vbox: Move parts of vboxSnapshotRedefine() into a separate function meson: Drop workaround for -Wframe-larger-than and clang meson.build | 5 - src/remote/remote_driver.c | 182 ++++--- src/vbox/vbox_common.c | 946 ++++++++++++++++++++----------------- 3 files changed, 636 insertions(+), 497 deletions(-) -- 2.49.0
From: Michal Privoznik <mprivozn@redhat.com> There's a problem with glib: what we might consider functions are in fact macros and to make things worse - they do declare local variables. For instance here's the declaration of g_clear_pointer() macro: #define g_clear_pointer(pp, destroy) \ G_STMT_START \ { \ G_STATIC_ASSERT (sizeof *(pp) == sizeof (gpointer)); \ glib_typeof ((pp)) _pp = (pp); \ glib_typeof (*(pp)) _ptr = *_pp; \ *_pp = NULL; \ if (_ptr) \ (destroy) (_ptr); \ } \ G_STMT_END \ Now, as of v6.2.0-rc1~267 our VIR_FREE() macro is in fact a redeclaration of g_clear_pointer(). Thus, calling VIR_FREE() increases stack size! Ideally, this wouldn't be a problem, because those variables (_pp, _ptr) live in their own block. And clever compiler can just reuse space created for one block. But then there's clang where we are hitting this exact problem in functions like doRemoteOpen() where either g_clear_pointer() is called directly, or there are macros like EXTRACT_URI_ARG_STR() which hide the call away. That's why despite our previous efforts decreasing stack size we still needed v9.8.0-rc1~208. Well, moving URI argument extraction (those calls to EXTRACT_URI_ARG_* macros) into a separate function helps us decrease stack size from 2296 bytes to 1320. Even after this there are still more possibilities for improvements, but those will be addressed in future commits. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/remote/remote_driver.c | 113 +++++++++++++++++++++++++++---------- 1 file changed, 82 insertions(+), 31 deletions(-) diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index 745cd34f83..681594e406 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -758,13 +758,74 @@ remoteConnectFormatURI(virURI *uri, virReportError(VIR_ERR_INVALID_ARG, \ _("Failed to parse value of URI component %1$s"), \ var->name); \ - goto error; \ + return -1; \ } \ ARG_VAR = tmp == 0; \ var->ignore = 1; \ continue; \ } +static int +doRemoteOpenExtractURIArgs(virConnectPtr conn, + char **name, + char **command, + char **sockname, + char **authtype, + char **sshauth, + char **netcat, + char **keyfile, + char **pkipath, + char **knownHosts, + char **knownHostsVerify, + char **tls_priority, + char **mode_str, + char **proxy_str, +#ifndef WIN32 + bool *tty, +#endif + bool *sanity, + bool *verify) +{ + size_t i; + + for (i = 0; i < conn->uri->paramsCount; i++) { + virURIParam *var = &conn->uri->params[i]; + + EXTRACT_URI_ARG_STR("name", *name); + EXTRACT_URI_ARG_STR("command", *command); + EXTRACT_URI_ARG_STR("socket", *sockname); + EXTRACT_URI_ARG_STR("auth", *authtype); + EXTRACT_URI_ARG_STR("sshauth", *sshauth); + EXTRACT_URI_ARG_STR("netcat", *netcat); + EXTRACT_URI_ARG_STR("keyfile", *keyfile); + EXTRACT_URI_ARG_STR("pkipath", *pkipath); + EXTRACT_URI_ARG_STR("known_hosts", *knownHosts); + EXTRACT_URI_ARG_STR("known_hosts_verify", *knownHostsVerify); + EXTRACT_URI_ARG_STR("tls_priority", *tls_priority); + EXTRACT_URI_ARG_STR("mode", *mode_str); + EXTRACT_URI_ARG_STR("proxy", *proxy_str); + EXTRACT_URI_ARG_BOOL("no_sanity", *sanity); + EXTRACT_URI_ARG_BOOL("no_verify", *verify); +#ifndef WIN32 + EXTRACT_URI_ARG_BOOL("no_tty", *tty); +#endif + + if (STRCASEEQ(var->name, "authfile")) { + /* Strip this param, used by virauth.c */ + var->ignore = 1; + continue; + } + + VIR_DEBUG("passing through variable '%s' ('%s') to remote end", + var->name, var->value); + } + + return 0; +} + +#undef EXTRACT_URI_ARG_STR +#undef EXTRACT_URI_ARG_BOOL + /* * URIs that this driver needs to handle: @@ -818,7 +879,6 @@ doRemoteOpen(virConnectPtr conn, bool tty = true; #endif int mode; - size_t i; int proxy; /* We handle *ALL* URIs here. The caller has rejected any @@ -844,35 +904,28 @@ doRemoteOpen(virConnectPtr conn, * although that won't be the case for now). */ if (conn->uri) { - for (i = 0; i < conn->uri->paramsCount; i++) { - virURIParam *var = &conn->uri->params[i]; - EXTRACT_URI_ARG_STR("name", name); - EXTRACT_URI_ARG_STR("command", command); - EXTRACT_URI_ARG_STR("socket", sockname); - EXTRACT_URI_ARG_STR("auth", authtype); - EXTRACT_URI_ARG_STR("sshauth", sshauth); - EXTRACT_URI_ARG_STR("netcat", netcat); - EXTRACT_URI_ARG_STR("keyfile", keyfile); - EXTRACT_URI_ARG_STR("pkipath", pkipath); - EXTRACT_URI_ARG_STR("known_hosts", knownHosts); - EXTRACT_URI_ARG_STR("known_hosts_verify", knownHostsVerify); - EXTRACT_URI_ARG_STR("tls_priority", tls_priority); - EXTRACT_URI_ARG_STR("mode", mode_str); - EXTRACT_URI_ARG_STR("proxy", proxy_str); - EXTRACT_URI_ARG_BOOL("no_sanity", sanity); - EXTRACT_URI_ARG_BOOL("no_verify", verify); + /* This really needs to be a separate function to keep + * the stack size at sane levels. */ + if (doRemoteOpenExtractURIArgs(conn, + &name, + &command, + &sockname, + &authtype, + &sshauth, + &netcat, + &keyfile, + &pkipath, + &knownHosts, + &knownHostsVerify, + &tls_priority, + &mode_str, + &proxy_str, #ifndef WIN32 - EXTRACT_URI_ARG_BOOL("no_tty", tty); + &tty, #endif - - if (STRCASEEQ(var->name, "authfile")) { - /* Strip this param, used by virauth.c */ - var->ignore = 1; - continue; - } - - VIR_DEBUG("passing through variable '%s' ('%s') to remote end", - var->name, var->value); + &sanity, + &verify) < 0) { + goto error; } /* Construct the original name. */ @@ -1248,8 +1301,6 @@ doRemoteOpen(virConnectPtr conn, VIR_FREE(priv->hostname); return VIR_DRV_OPEN_ERROR; } -#undef EXTRACT_URI_ARG_STR -#undef EXTRACT_URI_ARG_BOOL static struct private_data * remoteAllocPrivateData(void) -- 2.49.0
From: Michal Privoznik <mprivozn@redhat.com> When opening a connection, the client does some RPC talk (most notably REMOTE_PROC_CONNECT_OPEN, and in some cases REMOTE_PROC_CONNECT_GET_URI even). Now, calling RPC means that local variables must be created. Having them in doRemoteOpen() increases its stack size which goes against our effort in bringing the size down (see one of previous commits). Move that part of the code into a separate function. This brings the stack size of doRemoteOpen() even further: from 1320 bytes to 1272. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/remote/remote_driver.c | 65 ++++++++++++++++++++++---------------- 1 file changed, 38 insertions(+), 27 deletions(-) diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index 681594e406..3ecef7d73f 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -742,6 +742,42 @@ remoteConnectFormatURI(virURI *uri, } +static int +remoteCallOpen(virConnectPtr conn, + struct private_data *priv, + const char *name, + unsigned int flags) +{ + remote_connect_open_args args = { (char**) &name, flags }; + + VIR_DEBUG("Trying to open URI '%s'", name); + if (call(conn, priv, 0, REMOTE_PROC_CONNECT_OPEN, + (xdrproc_t) xdr_remote_connect_open_args, (char *) &args, + (xdrproc_t) xdr_void, (char *) NULL) == -1) + return -1; + + /* Now try and find out what URI the daemon used */ + if (conn->uri == NULL) { + remote_connect_get_uri_ret uriret = { 0 }; + + VIR_DEBUG("Trying to query remote URI"); + if (call(conn, priv, 0, + REMOTE_PROC_CONNECT_GET_URI, + (xdrproc_t) xdr_void, (char *) NULL, + (xdrproc_t) xdr_remote_connect_get_uri_ret, (char *) &uriret) < 0) + return -1; + + VIR_DEBUG("Auto-probed URI is %s", uriret.uri); + conn->uri = virURIParse(uriret.uri); + VIR_FREE(uriret.uri); + if (!conn->uri) + return -1; + } + + return 0; +} + + /* helper macro to ease extraction of arguments from the URI */ #define EXTRACT_URI_ARG_STR(ARG_NAME, ARG_VAR) \ if (STRCASEEQ(var->name, ARG_NAME)) { \ @@ -1241,33 +1277,8 @@ doRemoteOpen(virConnectPtr conn, } /* Finally we can call the remote side's open function. */ - { - remote_connect_open_args args = { &name, flags }; - - VIR_DEBUG("Trying to open URI '%s'", name); - if (call(conn, priv, 0, REMOTE_PROC_CONNECT_OPEN, - (xdrproc_t) xdr_remote_connect_open_args, (char *) &args, - (xdrproc_t) xdr_void, (char *) NULL) == -1) - goto error; - } - - /* Now try and find out what URI the daemon used */ - if (conn->uri == NULL) { - remote_connect_get_uri_ret uriret = { 0 }; - - VIR_DEBUG("Trying to query remote URI"); - if (call(conn, priv, 0, - REMOTE_PROC_CONNECT_GET_URI, - (xdrproc_t) xdr_void, (char *) NULL, - (xdrproc_t) xdr_remote_connect_get_uri_ret, (char *) &uriret) < 0) - goto error; - - VIR_DEBUG("Auto-probed URI is %s", uriret.uri); - conn->uri = virURIParse(uriret.uri); - VIR_FREE(uriret.uri); - if (!conn->uri) - goto error; - } + if (remoteCallOpen(conn, priv, name, flags) < 0) + goto error; /* Set up events */ if (!(priv->eventState = virObjectEventStateNew())) -- 2.49.0
From: Michal Privoznik <mprivozn@redhat.com> The @transport variable is already pass into the function with proper type. There's no need to typecast it to its very same type inside the function. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/remote/remote_driver.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index 3ecef7d73f..2690c05267 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -1063,7 +1063,7 @@ doRemoteOpen(virConnectPtr conn, VIR_DEBUG("Connecting with transport %d", transport); - switch ((remoteDriverTransport)transport) { + switch (transport) { case REMOTE_DRIVER_TRANSPORT_UNIX: case REMOTE_DRIVER_TRANSPORT_SSH: case REMOTE_DRIVER_TRANSPORT_LIBSSH: @@ -1088,7 +1088,7 @@ doRemoteOpen(virConnectPtr conn, VIR_DEBUG("Chosen UNIX socket %s", NULLSTR(sockname)); /* Connect to the remote service. */ - switch ((remoteDriverTransport)transport) { + switch (transport) { case REMOTE_DRIVER_TRANSPORT_TLS: if (conf && !tls_priority && virConfGetValueString(conf, "tls_priority", &tls_priority) < 0) -- 2.49.0
From: Michal Privoznik <mprivozn@redhat.com> There's too much happening inside of vboxSnapshotRedefine(). Not only it makes the function hard to read, but it also increases stack size of the function. Move one part into a separate function: vboxSnapshotReplaceRWDisks() Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/vbox/vbox_common.c | 216 ++++++++++++++++++++++------------------- 1 file changed, 118 insertions(+), 98 deletions(-) diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c index 7d157e065e..b52e0c3e6a 100644 --- a/src/vbox/vbox_common.c +++ b/src/vbox/vbox_common.c @@ -4553,6 +4553,122 @@ static int vboxCloseDisksRecursively(virDomainPtr dom, char *location) return ret; } + +static int +vboxSnapshotReplaceRWDisks(struct _vboxDriver *data, + virVBoxSnapshotConfMachine *snapshotMachineDesc, + char *currentSnapshotXmlFilePath) +{ + g_auto(GStrv) realReadWriteDisksPath = NULL; + g_auto(GStrv) realReadOnlyDisksPath = NULL; + int realReadWriteDisksPathSize = 0; + int realReadOnlyDisksPathSize = 0; + int it = 0; + + /* + * We have created fake disks, so we have to remove them and replace them with + * the read-write disks if there are any. The fake disks will be closed during + * the machine unregistration. + */ + if (virVBoxSnapshotConfRemoveFakeDisks(snapshotMachineDesc) < 0) { + vboxReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Unable to remove Fake Disks")); + return -1; + } + + realReadWriteDisksPathSize = virVBoxSnapshotConfGetRWDisksPathsFromLibvirtXML(currentSnapshotXmlFilePath, + &realReadWriteDisksPath); + realReadOnlyDisksPathSize = virVBoxSnapshotConfGetRODisksPathsFromLibvirtXML(currentSnapshotXmlFilePath, + &realReadOnlyDisksPath); + /* The read-only disk number is necessarily greater or equal to the + * read-write disk number */ + if (realReadOnlyDisksPathSize < realReadWriteDisksPathSize) { + vboxReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("The read only disk number must be greater or equal to the read write disk number")); + return -1; + } + + for (it = 0; it < realReadWriteDisksPathSize; it++) { + virVBoxSnapshotConfHardDisk *readWriteDisk = NULL; + PRUnichar *locationUtf = NULL; + IMedium *readWriteMedium = NULL; + char *uuid = NULL; + PRUnichar *formatUtf = NULL; + char *format = NULL; + const char *parentUuid = NULL; + vboxIID iid; + nsresult rc; + + VBOX_IID_INITIALIZE(&iid); + VBOX_UTF8_TO_UTF16(realReadWriteDisksPath[it], &locationUtf); + rc = gVBoxAPI.UIVirtualBox.OpenMedium(data->vboxObj, + locationUtf, + DeviceType_HardDisk, + AccessMode_ReadWrite, + &readWriteMedium); + if (NS_FAILED(rc)) { + vboxReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Unable to open HardDisk")); + VBOX_UTF16_FREE(locationUtf); + return -1; + } + VBOX_UTF16_FREE(locationUtf); + + rc = gVBoxAPI.UIMedium.GetId(readWriteMedium, &iid); + if (NS_FAILED(rc)) { + vboxReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Unable to get the read write medium id")); + return -1; + } + gVBoxAPI.UIID.vboxIIDToUtf8(data, &iid, &uuid); + vboxIIDUnalloc(&iid); + + rc = gVBoxAPI.UIMedium.GetFormat(readWriteMedium, &formatUtf); + if (NS_FAILED(rc)) { + vboxReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Unable to get the read write medium format")); + return -1; + } + VBOX_UTF16_TO_UTF8(formatUtf, &format); + VBOX_UTF16_FREE(formatUtf); + + readWriteDisk = g_new0(virVBoxSnapshotConfHardDisk, 1); + + readWriteDisk->format = format; + readWriteDisk->uuid = uuid; + readWriteDisk->location = realReadWriteDisksPath[it]; + /* + * We get the current snapshot's read-only disk uuid in order to add the + * read-write disk to the media registry as its child. The read-only disk + * is already in the media registry because it is the fake disk's parent. + */ + parentUuid = virVBoxSnapshotConfHardDiskUuidByLocation(snapshotMachineDesc, + realReadOnlyDisksPath[it]); + if (parentUuid == NULL) { + VIR_FREE(readWriteDisk); + return -1; + } + + if (virVBoxSnapshotConfAddHardDiskToMediaRegistry(readWriteDisk, + snapshotMachineDesc->mediaRegistry, + parentUuid) < 0) { + vboxReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Unable to add hard disk to media Registry")); + VIR_FREE(readWriteDisk); + return -1; + } + rc = gVBoxAPI.UIMedium.Close(readWriteMedium); + if (NS_FAILED(rc)) { + vboxReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Unable to close HardDisk")); + return -1; + } + } + + return 0; +} + + static int vboxSnapshotRedefine(virDomainPtr dom, virDomainSnapshotDef *def, @@ -4596,10 +4712,6 @@ vboxSnapshotRedefine(virDomainPtr dom, char *currentSnapshotXmlFilePath = NULL; PRUnichar *machineNameUtf16 = NULL; char *machineName = NULL; - g_auto(GStrv) realReadWriteDisksPath = NULL; - int realReadWriteDisksPathSize = 0; - g_auto(GStrv) realReadOnlyDisksPath = NULL; - int realReadOnlyDisksPathSize = 0; virVBoxSnapshotConfSnapshot *newSnapshotPtr = NULL; unsigned char snapshotUuid[VIR_UUID_BUFLEN]; virVBoxSnapshotConfHardDisk **hardDiskToOpen = NULL; @@ -4668,102 +4780,10 @@ vboxSnapshotRedefine(virDomainPtr dom, } if (snapshotFileExists) { - /* - * We have created fake disks, so we have to remove them and replace them with - * the read-write disks if there are any. The fake disks will be closed during - * the machine unregistration. - */ - if (virVBoxSnapshotConfRemoveFakeDisks(snapshotMachineDesc) < 0) { - vboxReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Unable to remove Fake Disks")); + if (vboxSnapshotReplaceRWDisks(data, snapshotMachineDesc, + currentSnapshotXmlFilePath) < 0) goto cleanup; - } - realReadWriteDisksPathSize = virVBoxSnapshotConfGetRWDisksPathsFromLibvirtXML(currentSnapshotXmlFilePath, - &realReadWriteDisksPath); - realReadOnlyDisksPathSize = virVBoxSnapshotConfGetRODisksPathsFromLibvirtXML(currentSnapshotXmlFilePath, - &realReadOnlyDisksPath); - /* The read-only disk number is necessarily greater or equal to the - * read-write disk number */ - if (realReadOnlyDisksPathSize < realReadWriteDisksPathSize) { - vboxReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("The read only disk number must be greater or equal to the read write disk number")); - goto cleanup; - } - for (it = 0; it < realReadWriteDisksPathSize; it++) { - virVBoxSnapshotConfHardDisk *readWriteDisk = NULL; - PRUnichar *locationUtf = NULL; - IMedium *readWriteMedium = NULL; - char *uuid = NULL; - PRUnichar *formatUtf = NULL; - char *format = NULL; - const char *parentUuid = NULL; - vboxIID iid; - - VBOX_IID_INITIALIZE(&iid); - VBOX_UTF8_TO_UTF16(realReadWriteDisksPath[it], &locationUtf); - rc = gVBoxAPI.UIVirtualBox.OpenMedium(data->vboxObj, - locationUtf, - DeviceType_HardDisk, - AccessMode_ReadWrite, - &readWriteMedium); - if (NS_FAILED(rc)) { - vboxReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Unable to open HardDisk")); - VBOX_UTF16_FREE(locationUtf); - goto cleanup; - } - VBOX_UTF16_FREE(locationUtf); - - rc = gVBoxAPI.UIMedium.GetId(readWriteMedium, &iid); - if (NS_FAILED(rc)) { - vboxReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Unable to get the read write medium id")); - goto cleanup; - } - gVBoxAPI.UIID.vboxIIDToUtf8(data, &iid, &uuid); - vboxIIDUnalloc(&iid); - - rc = gVBoxAPI.UIMedium.GetFormat(readWriteMedium, &formatUtf); - if (NS_FAILED(rc)) { - vboxReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Unable to get the read write medium format")); - goto cleanup; - } - VBOX_UTF16_TO_UTF8(formatUtf, &format); - VBOX_UTF16_FREE(formatUtf); - - readWriteDisk = g_new0(virVBoxSnapshotConfHardDisk, 1); - - readWriteDisk->format = format; - readWriteDisk->uuid = uuid; - readWriteDisk->location = realReadWriteDisksPath[it]; - /* - * We get the current snapshot's read-only disk uuid in order to add the - * read-write disk to the media registry as its child. The read-only disk - * is already in the media registry because it is the fake disk's parent. - */ - parentUuid = virVBoxSnapshotConfHardDiskUuidByLocation(snapshotMachineDesc, - realReadOnlyDisksPath[it]); - if (parentUuid == NULL) { - VIR_FREE(readWriteDisk); - goto cleanup; - } - if (virVBoxSnapshotConfAddHardDiskToMediaRegistry(readWriteDisk, - snapshotMachineDesc->mediaRegistry, - parentUuid) < 0) { - vboxReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Unable to add hard disk to media Registry")); - VIR_FREE(readWriteDisk); - goto cleanup; - } - rc = gVBoxAPI.UIMedium.Close(readWriteMedium); - if (NS_FAILED(rc)) { - vboxReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Unable to close HardDisk")); - goto cleanup; - } - } /* * Now we have done this swap, we remove the snapshot xml file from the * current machine location. -- 2.49.0
From: Michal Privoznik <mprivozn@redhat.com> There's too much happening inside of vboxSnapshotRedefine(). Not only it makes the function hard to read, but it also increases stack size of the function. Move one part into a separate function: vboxSnapshotAddDisksToMediaRegistry() Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/vbox/vbox_common.c | 215 ++++++++++++++++++++++------------------- 1 file changed, 115 insertions(+), 100 deletions(-) diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c index b52e0c3e6a..e5490304d8 100644 --- a/src/vbox/vbox_common.c +++ b/src/vbox/vbox_common.c @@ -4669,6 +4669,119 @@ vboxSnapshotReplaceRWDisks(struct _vboxDriver *data, } +static int +vboxSnapshotAddDisksToMediaRegistry(struct _vboxDriver *data, + virDomainSnapshotDef *def, + virVBoxSnapshotConfMachine *snapshotMachineDesc) +{ + int it = 0; + + for (it = 0; it < def->parent.dom->ndisks; it++) { + int diskInMediaRegistry = 0; + IMedium *readOnlyMedium = NULL; + PRUnichar *locationUtf = NULL; + char *uuid = NULL; + PRUnichar *formatUtf = NULL; + char *format = NULL; + char *parentUuid = NULL; + virVBoxSnapshotConfHardDisk *readOnlyDisk = NULL; + vboxIID iid, parentiid; + IMedium *parentReadOnlyMedium = NULL; + nsresult rc; + + VBOX_IID_INITIALIZE(&iid); + VBOX_IID_INITIALIZE(&parentiid); + diskInMediaRegistry = virVBoxSnapshotConfDiskIsInMediaRegistry(snapshotMachineDesc, + def->parent.dom->disks[it]->src->path); + if (diskInMediaRegistry == -1) { + vboxReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Unable to know if disk is in media registry")); + return -1; + } + if (diskInMediaRegistry == 1) /* Nothing to do. */ + continue; + /* The read only disk is not in the media registry */ + + VBOX_UTF8_TO_UTF16(def->parent.dom->disks[it]->src->path, &locationUtf); + rc = gVBoxAPI.UIVirtualBox.OpenMedium(data->vboxObj, + locationUtf, + DeviceType_HardDisk, + AccessMode_ReadWrite, + &readOnlyMedium); + if (NS_FAILED(rc)) { + vboxReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Unable to open HardDisk")); + VBOX_UTF16_FREE(locationUtf); + return -1; + } + VBOX_UTF16_FREE(locationUtf); + + rc = gVBoxAPI.UIMedium.GetId(readOnlyMedium, &iid); + if (NS_FAILED(rc)) { + vboxReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Unable to get hard disk id")); + return -1; + } + gVBoxAPI.UIID.vboxIIDToUtf8(data, &iid, &uuid); + vboxIIDUnalloc(&iid); + + rc = gVBoxAPI.UIMedium.GetFormat(readOnlyMedium, &formatUtf); + if (NS_FAILED(rc)) { + vboxReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Unable to get hard disk format")); + VIR_FREE(uuid); + return -1; + } + VBOX_UTF16_TO_UTF8(formatUtf, &format); + VBOX_UTF16_FREE(formatUtf); + + /* This disk is already in the media registry */ + rc = gVBoxAPI.UIMedium.GetParent(readOnlyMedium, &parentReadOnlyMedium); + if (NS_FAILED(rc)) { + vboxReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Unable to get parent hard disk")); + VIR_FREE(uuid); + return -1; + } + + rc = gVBoxAPI.UIMedium.GetId(parentReadOnlyMedium, &parentiid); + if (NS_FAILED(rc)) { + vboxReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Unable to get hard disk id")); + VIR_FREE(uuid); + return -1; + } + gVBoxAPI.UIID.vboxIIDToUtf8(data, &parentiid, &parentUuid); + vboxIIDUnalloc(&parentiid); + + rc = gVBoxAPI.UIMedium.Close(readOnlyMedium); + if (NS_FAILED(rc)) { + vboxReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Unable to close HardDisk")); + VIR_FREE(uuid); + VIR_FREE(parentUuid); + return -1; + } + + readOnlyDisk = g_new0(virVBoxSnapshotConfHardDisk, 1); + + readOnlyDisk->format = format; + readOnlyDisk->uuid = uuid; + readOnlyDisk->location = g_strdup(def->parent.dom->disks[it]->src->path); + + if (virVBoxSnapshotConfAddHardDiskToMediaRegistry(readOnlyDisk, snapshotMachineDesc->mediaRegistry, + parentUuid) < 0) { + vboxReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Unable to add hard disk to media registry")); + VIR_FREE(readOnlyDisk); + return -1; + } + } + + return 0; +} + + static int vboxSnapshotRedefine(virDomainPtr dom, virDomainSnapshotDef *def, @@ -4799,106 +4912,8 @@ vboxSnapshotRedefine(virDomainPtr dom, * read-only disks are in the redefined snapshot's media registry (the disks need to * be open to query their uuid). */ - for (it = 0; it < def->parent.dom->ndisks; it++) { - int diskInMediaRegistry = 0; - IMedium *readOnlyMedium = NULL; - PRUnichar *locationUtf = NULL; - char *uuid = NULL; - PRUnichar *formatUtf = NULL; - char *format = NULL; - char *parentUuid = NULL; - virVBoxSnapshotConfHardDisk *readOnlyDisk = NULL; - vboxIID iid, parentiid; - IMedium *parentReadOnlyMedium = NULL; - - VBOX_IID_INITIALIZE(&iid); - VBOX_IID_INITIALIZE(&parentiid); - diskInMediaRegistry = virVBoxSnapshotConfDiskIsInMediaRegistry(snapshotMachineDesc, - def->parent.dom->disks[it]->src->path); - if (diskInMediaRegistry == -1) { - vboxReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Unable to know if disk is in media registry")); - goto cleanup; - } - if (diskInMediaRegistry == 1) /* Nothing to do. */ - continue; - /* The read only disk is not in the media registry */ - - VBOX_UTF8_TO_UTF16(def->parent.dom->disks[it]->src->path, &locationUtf); - rc = gVBoxAPI.UIVirtualBox.OpenMedium(data->vboxObj, - locationUtf, - DeviceType_HardDisk, - AccessMode_ReadWrite, - &readOnlyMedium); - if (NS_FAILED(rc)) { - vboxReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Unable to open HardDisk")); - VBOX_UTF16_FREE(locationUtf); - goto cleanup; - } - VBOX_UTF16_FREE(locationUtf); - - rc = gVBoxAPI.UIMedium.GetId(readOnlyMedium, &iid); - if (NS_FAILED(rc)) { - vboxReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Unable to get hard disk id")); - goto cleanup; - } - gVBoxAPI.UIID.vboxIIDToUtf8(data, &iid, &uuid); - vboxIIDUnalloc(&iid); - - rc = gVBoxAPI.UIMedium.GetFormat(readOnlyMedium, &formatUtf); - if (NS_FAILED(rc)) { - vboxReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Unable to get hard disk format")); - VIR_FREE(uuid); - goto cleanup; - } - VBOX_UTF16_TO_UTF8(formatUtf, &format); - VBOX_UTF16_FREE(formatUtf); - - /* This disk is already in the media registry */ - rc = gVBoxAPI.UIMedium.GetParent(readOnlyMedium, &parentReadOnlyMedium); - if (NS_FAILED(rc)) { - vboxReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Unable to get parent hard disk")); - VIR_FREE(uuid); - goto cleanup; - } - - rc = gVBoxAPI.UIMedium.GetId(parentReadOnlyMedium, &parentiid); - if (NS_FAILED(rc)) { - vboxReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Unable to get hard disk id")); - VIR_FREE(uuid); - goto cleanup; - } - gVBoxAPI.UIID.vboxIIDToUtf8(data, &parentiid, &parentUuid); - vboxIIDUnalloc(&parentiid); - - rc = gVBoxAPI.UIMedium.Close(readOnlyMedium); - if (NS_FAILED(rc)) { - vboxReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Unable to close HardDisk")); - VIR_FREE(uuid); - VIR_FREE(parentUuid); - goto cleanup; - } - - readOnlyDisk = g_new0(virVBoxSnapshotConfHardDisk, 1); - - readOnlyDisk->format = format; - readOnlyDisk->uuid = uuid; - readOnlyDisk->location = g_strdup(def->parent.dom->disks[it]->src->path); - - if (virVBoxSnapshotConfAddHardDiskToMediaRegistry(readOnlyDisk, snapshotMachineDesc->mediaRegistry, - parentUuid) < 0) { - vboxReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Unable to add hard disk to media registry")); - VIR_FREE(readOnlyDisk); - goto cleanup; - } - } + if (vboxSnapshotAddDisksToMediaRegistry(data, def, snapshotMachineDesc) < 0) + goto cleanup; /* Now, we can unregister the machine */ rc = gVBoxAPI.UIMachine.Unregister(machine, -- 2.49.0
From: Michal Privoznik <mprivozn@redhat.com> There's too much happening inside of vboxSnapshotRedefine(). Not only it makes the function hard to read, but it also increases stack size of the function. Move one part into a separate function: vboxSnapshotAddRWDisks() Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/vbox/vbox_common.c | 227 ++++++++++++++++++++++------------------- 1 file changed, 124 insertions(+), 103 deletions(-) diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c index e5490304d8..9ed972e52e 100644 --- a/src/vbox/vbox_common.c +++ b/src/vbox/vbox_common.c @@ -4782,6 +4782,127 @@ vboxSnapshotAddDisksToMediaRegistry(struct _vboxDriver *data, } +static int +vboxSnapshotAddRWDisks(struct _vboxDriver *data, + virDomainSnapshotDef *def, + virVBoxSnapshotConfMachine *snapshotMachineDesc, + bool needToChangeStorageController) +{ + int it = 0; + + for (it = 0; it < def->ndisks; it++) { + IMedium *medium = NULL; + PRUnichar *locationUtf16 = NULL; + virVBoxSnapshotConfHardDisk *disk = NULL; + PRUnichar *formatUtf16 = NULL; + char *format = NULL; + char *uuid = NULL; + IMedium *parentDisk = NULL; + char *parentUuid = NULL; + vboxIID iid, parentiid; + nsresult rc; + + VBOX_IID_INITIALIZE(&iid); + VBOX_IID_INITIALIZE(&parentiid); + VBOX_UTF8_TO_UTF16(def->disks[it].src->path, &locationUtf16); + rc = gVBoxAPI.UIVirtualBox.OpenMedium(data->vboxObj, + locationUtf16, + DeviceType_HardDisk, + AccessMode_ReadWrite, + &medium); + if (NS_FAILED(rc)) { + vboxReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Unable to open HardDisk")); + return -1; + } + VBOX_UTF16_FREE(locationUtf16); + + disk = g_new0(virVBoxSnapshotConfHardDisk, 1); + + rc = gVBoxAPI.UIMedium.GetFormat(medium, &formatUtf16); + if (NS_FAILED(rc)) { + vboxReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Unable to get disk format")); + VIR_FREE(disk); + return -1; + } + + VBOX_UTF16_TO_UTF8(formatUtf16, &format); + disk->format = format; + VBOX_UTF16_FREE(formatUtf16); + + disk->location = g_strdup(def->disks[it].src->path); + + rc = gVBoxAPI.UIMedium.GetId(medium, &iid); + if (NS_FAILED(rc)) { + vboxReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Unable to get disk uuid")); + VIR_FREE(disk); + return -1; + } + gVBoxAPI.UIID.vboxIIDToUtf8(data, &iid, &uuid); + disk->uuid = uuid; + vboxIIDUnalloc(&iid); + + rc = gVBoxAPI.UIMedium.GetParent(medium, &parentDisk); + if (NS_FAILED(rc)) { + vboxReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Unable to get disk parent")); + VIR_FREE(disk); + return -1; + } + + gVBoxAPI.UIMedium.GetId(parentDisk, &parentiid); + gVBoxAPI.UIID.vboxIIDToUtf8(data, &parentiid, &parentUuid); + vboxIIDUnalloc(&parentiid); + if (virVBoxSnapshotConfAddHardDiskToMediaRegistry(disk, + snapshotMachineDesc->mediaRegistry, + parentUuid) < 0) { + vboxReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Unable to add hard disk to the media registry")); + VIR_FREE(disk); + return -1; + } + + if (needToChangeStorageController) { + /* We need to append this disk in the storage controller */ + g_auto(GStrv) searchResultTab = NULL; + char *tmp = NULL; + ssize_t resultSize = 0; + + resultSize = virStringSearch(snapshotMachineDesc->storageController, + VBOX_UUID_REGEX, + it + 1, + &searchResultTab); + if (resultSize != it + 1) { + vboxReportError(VIR_ERR_INTERNAL_ERROR, + _("Unable to find UUID %1$s"), searchResultTab[it]); + return -1; + } + + tmp = virStringReplace(snapshotMachineDesc->storageController, + searchResultTab[it], + disk->uuid); + VIR_FREE(snapshotMachineDesc->storageController); + if (!tmp) + return -1; + snapshotMachineDesc->storageController = g_strdup(tmp); + + VIR_FREE(tmp); + } + /* Close disk */ + rc = gVBoxAPI.UIMedium.Close(medium); + if (NS_FAILED(rc)) { + vboxReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Unable to close HardDisk")); + return -1; + } + } + + return 0; +} + + static int vboxSnapshotRedefine(virDomainPtr dom, virDomainSnapshotDef *def, @@ -5097,109 +5218,9 @@ vboxSnapshotRedefine(virDomainPtr dom, * If the snapshot to redefine is the current snapshot, we add read-write disks in * the machine storage controllers. */ - for (it = 0; it < def->ndisks; it++) { - IMedium *medium = NULL; - PRUnichar *locationUtf16 = NULL; - virVBoxSnapshotConfHardDisk *disk = NULL; - PRUnichar *formatUtf16 = NULL; - char *format = NULL; - char *uuid = NULL; - IMedium *parentDisk = NULL; - char *parentUuid = NULL; - vboxIID iid, parentiid; - - VBOX_IID_INITIALIZE(&iid); - VBOX_IID_INITIALIZE(&parentiid); - VBOX_UTF8_TO_UTF16(def->disks[it].src->path, &locationUtf16); - rc = gVBoxAPI.UIVirtualBox.OpenMedium(data->vboxObj, - locationUtf16, - DeviceType_HardDisk, - AccessMode_ReadWrite, - &medium); - if (NS_FAILED(rc)) { - vboxReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Unable to open HardDisk")); - goto cleanup; - } - VBOX_UTF16_FREE(locationUtf16); - - disk = g_new0(virVBoxSnapshotConfHardDisk, 1); - - rc = gVBoxAPI.UIMedium.GetFormat(medium, &formatUtf16); - if (NS_FAILED(rc)) { - vboxReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Unable to get disk format")); - VIR_FREE(disk); - goto cleanup; - } - - VBOX_UTF16_TO_UTF8(formatUtf16, &format); - disk->format = format; - VBOX_UTF16_FREE(formatUtf16); - - disk->location = g_strdup(def->disks[it].src->path); - - rc = gVBoxAPI.UIMedium.GetId(medium, &iid); - if (NS_FAILED(rc)) { - vboxReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Unable to get disk uuid")); - VIR_FREE(disk); - goto cleanup; - } - gVBoxAPI.UIID.vboxIIDToUtf8(data, &iid, &uuid); - disk->uuid = uuid; - vboxIIDUnalloc(&iid); - - rc = gVBoxAPI.UIMedium.GetParent(medium, &parentDisk); - if (NS_FAILED(rc)) { - vboxReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Unable to get disk parent")); - VIR_FREE(disk); - goto cleanup; - } - - gVBoxAPI.UIMedium.GetId(parentDisk, &parentiid); - gVBoxAPI.UIID.vboxIIDToUtf8(data, &parentiid, &parentUuid); - vboxIIDUnalloc(&parentiid); - if (virVBoxSnapshotConfAddHardDiskToMediaRegistry(disk, - snapshotMachineDesc->mediaRegistry, - parentUuid) < 0) { - vboxReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Unable to add hard disk to the media registry")); - VIR_FREE(disk); - goto cleanup; - } - - if (needToChangeStorageController) { - /* We need to append this disk in the storage controller */ - char *tmp = NULL; - resultSize = virStringSearch(snapshotMachineDesc->storageController, - VBOX_UUID_REGEX, - it + 1, - &searchResultTab); - if (resultSize != it + 1) { - vboxReportError(VIR_ERR_INTERNAL_ERROR, - _("Unable to find UUID %1$s"), searchResultTab[it]); - goto cleanup; - } - - tmp = virStringReplace(snapshotMachineDesc->storageController, - searchResultTab[it], - disk->uuid); - VIR_FREE(snapshotMachineDesc->storageController); - if (!tmp) - goto cleanup; - snapshotMachineDesc->storageController = g_strdup(tmp); - - VIR_FREE(tmp); - } - /* Close disk */ - rc = gVBoxAPI.UIMedium.Close(medium); - if (NS_FAILED(rc)) { - vboxReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Unable to close HardDisk")); - goto cleanup; - } + if (vboxSnapshotAddRWDisks(data, def, snapshotMachineDesc, + needToChangeStorageController) < 0) { + goto cleanup; } } else { char *snapshotContent; -- 2.49.0
From: Michal Privoznik <mprivozn@redhat.com> There's too much happening inside of vboxSnapshotRedefine(). Not only it makes the function hard to read, but it also increases stack size of the function. Move one part into a separate function: vboxSnapshotCreateFakeDiffStorage() Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/vbox/vbox_common.c | 288 ++++++++++++++++++++++------------------- 1 file changed, 157 insertions(+), 131 deletions(-) diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c index 9ed972e52e..349ac832dc 100644 --- a/src/vbox/vbox_common.c +++ b/src/vbox/vbox_common.c @@ -4903,6 +4903,157 @@ vboxSnapshotAddRWDisks(struct _vboxDriver *data, } +static int +vboxSnapshotCreateFakeDiffStorage(struct _vboxDriver *data, + virDomainSnapshotDef *def, + char *machineLocationPath, + virVBoxSnapshotConfMachine *snapshotMachineDesc) +{ + virVBoxSnapshotConfHardDisk *newHardDisk = NULL; + int it; + int ret = -1; + + for (it = 0; it < def->parent.dom->ndisks; it++) { + IMedium *medium = NULL; + PRUnichar *locationUtf16 = NULL; + char *parentUuid = NULL; + IMedium *newMedium = NULL; + PRUnichar *formatUtf16 = NULL; + PRUnichar *newLocation = NULL; + char *newLocationUtf8 = NULL; + resultCodeUnion resultCode; + char *uuid = NULL; + char *format = NULL; + char *tmp = NULL; + vboxIID iid, parentiid; + IProgress *progress = NULL; + PRUint32 tab[1]; + nsresult rc; + g_auto(GStrv) searchResultTab = NULL; + ssize_t resultSize = 0; + + VBOX_IID_INITIALIZE(&iid); + VBOX_IID_INITIALIZE(&parentiid); + VBOX_UTF8_TO_UTF16(def->parent.dom->disks[it]->src->path, &locationUtf16); + rc = gVBoxAPI.UIVirtualBox.OpenMedium(data->vboxObj, + locationUtf16, + DeviceType_HardDisk, + AccessMode_ReadWrite, + &medium); + if (NS_FAILED(rc)) { + vboxReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Unable to open HardDisk")); + VBOX_UTF16_FREE(locationUtf16); + goto cleanup; + } + VBOX_UTF16_FREE(locationUtf16); + + rc = gVBoxAPI.UIMedium.GetId(medium, &parentiid); + if (NS_FAILED(rc)) { + vboxReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Unable to get hard disk id")); + goto cleanup; + } + gVBoxAPI.UIID.vboxIIDToUtf8(data, &parentiid, &parentUuid); + vboxIIDUnalloc(&parentiid); + VBOX_UTF8_TO_UTF16("VDI", &formatUtf16); + + newLocationUtf8 = g_strdup_printf("%sfakedisk-%d.vdi", + machineLocationPath, it); + VBOX_UTF8_TO_UTF16(newLocationUtf8, &newLocation); + rc = gVBoxAPI.UIVirtualBox.CreateHardDisk(data->vboxObj, + formatUtf16, + newLocation, + &newMedium); + VBOX_UTF16_FREE(newLocation); + VBOX_UTF16_FREE(formatUtf16); + if (NS_FAILED(rc)) { + vboxReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Unable to create HardDisk")); + goto cleanup; + } + + tab[0] = MediumVariant_Diff; + gVBoxAPI.UIMedium.CreateDiffStorage(medium, newMedium, 1, tab, &progress); + + gVBoxAPI.UIProgress.WaitForCompletion(progress, -1); + gVBoxAPI.UIProgress.GetResultCode(progress, &resultCode); + if (RC_FAILED(resultCode)) { + vboxReportError(VIR_ERR_INTERNAL_ERROR, + _("Error while creating diff storage, rc=%1$08x"), + resultCode.uResultCode); + goto cleanup; + } + VBOX_RELEASE(progress); + /* + * The differential newHardDisk is created, we add it to the + * media registry and the machine storage controllers. + */ + + newHardDisk = g_new0(virVBoxSnapshotConfHardDisk, 1); + + rc = gVBoxAPI.UIMedium.GetId(newMedium, &iid); + if (NS_FAILED(rc)) { + vboxReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Unable to get medium uuid")); + goto cleanup; + } + gVBoxAPI.UIID.vboxIIDToUtf8(data, &iid, &uuid); + newHardDisk->uuid = uuid; + vboxIIDUnalloc(&iid); + + newHardDisk->location = g_strdup(newLocationUtf8); + + rc = gVBoxAPI.UIMedium.GetFormat(newMedium, &formatUtf16); + VBOX_UTF16_TO_UTF8(formatUtf16, &format); + newHardDisk->format = format; + VBOX_UTF16_FREE(formatUtf16); + + if (virVBoxSnapshotConfAddHardDiskToMediaRegistry(newHardDisk, + snapshotMachineDesc->mediaRegistry, + parentUuid) < 0) { + vboxReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Unable to add hard disk to the media registry")); + goto cleanup; + } + newHardDisk = NULL; /* Consumed by above */ + /* Adding the fake disk to the machine storage controllers */ + + resultSize = virStringSearch(snapshotMachineDesc->storageController, + VBOX_UUID_REGEX, + it + 1, + &searchResultTab); + if (resultSize != it + 1) { + vboxReportError(VIR_ERR_INTERNAL_ERROR, + _("Unable to find UUID %1$s"), searchResultTab[it]); + goto cleanup; + } + + tmp = virStringReplace(snapshotMachineDesc->storageController, + searchResultTab[it], + uuid); + VIR_FREE(snapshotMachineDesc->storageController); + if (!tmp) + goto cleanup; + snapshotMachineDesc->storageController = g_strdup(tmp); + + VIR_FREE(tmp); + /* Closing the "fake" disk */ + rc = gVBoxAPI.UIMedium.Close(newMedium); + if (NS_FAILED(rc)) { + vboxReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Unable to close the new medium")); + goto cleanup; + } + } + + ret = 0; + cleanup: + virVboxSnapshotConfHardDiskFree(newHardDisk); + return ret; +} + + static int vboxSnapshotRedefine(virDomainPtr dom, virDomainSnapshotDef *def, @@ -4950,7 +5101,6 @@ vboxSnapshotRedefine(virDomainPtr dom, unsigned char snapshotUuid[VIR_UUID_BUFLEN]; virVBoxSnapshotConfHardDisk **hardDiskToOpen = NULL; size_t hardDiskToOpenSize = 0; - virVBoxSnapshotConfHardDisk *newHardDisk = NULL; g_auto(GStrv) searchResultTab = NULL; ssize_t resultSize = 0; int it = 0; @@ -5224,137 +5374,14 @@ vboxSnapshotRedefine(virDomainPtr dom, } } else { char *snapshotContent; + /* Create a "fake" disk to avoid corrupting children snapshot disks. */ - for (it = 0; it < def->parent.dom->ndisks; it++) { - IMedium *medium = NULL; - PRUnichar *locationUtf16 = NULL; - char *parentUuid = NULL; - IMedium *newMedium = NULL; - PRUnichar *formatUtf16 = NULL; - PRUnichar *newLocation = NULL; - char *newLocationUtf8 = NULL; - resultCodeUnion resultCode; - char *uuid = NULL; - char *format = NULL; - char *tmp = NULL; - vboxIID iid, parentiid; - IProgress *progress = NULL; - PRUint32 tab[1]; - - VBOX_IID_INITIALIZE(&iid); - VBOX_IID_INITIALIZE(&parentiid); - VBOX_UTF8_TO_UTF16(def->parent.dom->disks[it]->src->path, &locationUtf16); - rc = gVBoxAPI.UIVirtualBox.OpenMedium(data->vboxObj, - locationUtf16, - DeviceType_HardDisk, - AccessMode_ReadWrite, - &medium); - if (NS_FAILED(rc)) { - vboxReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Unable to open HardDisk")); - VBOX_UTF16_FREE(locationUtf16); - goto cleanup; - } - VBOX_UTF16_FREE(locationUtf16); - - rc = gVBoxAPI.UIMedium.GetId(medium, &parentiid); - if (NS_FAILED(rc)) { - vboxReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Unable to get hard disk id")); - goto cleanup; - } - gVBoxAPI.UIID.vboxIIDToUtf8(data, &parentiid, &parentUuid); - vboxIIDUnalloc(&parentiid); - VBOX_UTF8_TO_UTF16("VDI", &formatUtf16); - - newLocationUtf8 = g_strdup_printf("%sfakedisk-%d.vdi", - machineLocationPath, it); - VBOX_UTF8_TO_UTF16(newLocationUtf8, &newLocation); - rc = gVBoxAPI.UIVirtualBox.CreateHardDisk(data->vboxObj, - formatUtf16, - newLocation, - &newMedium); - VBOX_UTF16_FREE(newLocation); - VBOX_UTF16_FREE(formatUtf16); - if (NS_FAILED(rc)) { - vboxReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Unable to create HardDisk")); - goto cleanup; - } - - tab[0] = MediumVariant_Diff; - gVBoxAPI.UIMedium.CreateDiffStorage(medium, newMedium, 1, tab, &progress); - - gVBoxAPI.UIProgress.WaitForCompletion(progress, -1); - gVBoxAPI.UIProgress.GetResultCode(progress, &resultCode); - if (RC_FAILED(resultCode)) { - vboxReportError(VIR_ERR_INTERNAL_ERROR, - _("Error while creating diff storage, rc=%1$08x"), - resultCode.uResultCode); - goto cleanup; - } - VBOX_RELEASE(progress); - /* - * The differential newHardDisk is created, we add it to the - * media registry and the machine storage controllers. - */ - - newHardDisk = g_new0(virVBoxSnapshotConfHardDisk, 1); - - rc = gVBoxAPI.UIMedium.GetId(newMedium, &iid); - if (NS_FAILED(rc)) { - vboxReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Unable to get medium uuid")); - goto cleanup; - } - gVBoxAPI.UIID.vboxIIDToUtf8(data, &iid, &uuid); - newHardDisk->uuid = uuid; - vboxIIDUnalloc(&iid); - - newHardDisk->location = g_strdup(newLocationUtf8); - - rc = gVBoxAPI.UIMedium.GetFormat(newMedium, &formatUtf16); - VBOX_UTF16_TO_UTF8(formatUtf16, &format); - newHardDisk->format = format; - VBOX_UTF16_FREE(formatUtf16); - - if (virVBoxSnapshotConfAddHardDiskToMediaRegistry(newHardDisk, - snapshotMachineDesc->mediaRegistry, - parentUuid) < 0) { - vboxReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Unable to add hard disk to the media registry")); - goto cleanup; - } - newHardDisk = NULL; /* Consumed by above */ - /* Adding the fake disk to the machine storage controllers */ - - resultSize = virStringSearch(snapshotMachineDesc->storageController, - VBOX_UUID_REGEX, - it + 1, - &searchResultTab); - if (resultSize != it + 1) { - vboxReportError(VIR_ERR_INTERNAL_ERROR, - _("Unable to find UUID %1$s"), searchResultTab[it]); - goto cleanup; - } - - tmp = virStringReplace(snapshotMachineDesc->storageController, - searchResultTab[it], - uuid); - VIR_FREE(snapshotMachineDesc->storageController); - if (!tmp) - goto cleanup; - snapshotMachineDesc->storageController = g_strdup(tmp); - - VIR_FREE(tmp); - /* Closing the "fake" disk */ - rc = gVBoxAPI.UIMedium.Close(newMedium); - if (NS_FAILED(rc)) { - vboxReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Unable to close the new medium")); - goto cleanup; - } + if (vboxSnapshotCreateFakeDiffStorage(data, def, + machineLocationPath, + snapshotMachineDesc) < 0) { + goto cleanup; } + /* * We save the snapshot xml file to retrieve the real read-write disk during the * next define. This file is saved as "'machineLocation'/snapshot-'uuid'.xml" @@ -5443,7 +5470,6 @@ vboxSnapshotRedefine(virDomainPtr dom, VIR_FREE(currentSnapshotXmlFilePath); VBOX_UTF16_FREE(machineNameUtf16); VBOX_UTF8_FREE(machineName); - virVboxSnapshotConfHardDiskFree(newHardDisk); VIR_FREE(hardDiskToOpen); VIR_FREE(newSnapshotPtr); VIR_FREE(machineLocationPath); -- 2.49.0
From: Michal Privoznik <mprivozn@redhat.com> After previous cleanups, all functions have their stack smaller than 2048 bytes and thus the workaround is no longer needed. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- meson.build | 5 ----- 1 file changed, 5 deletions(-) diff --git a/meson.build b/meson.build index bf4a245dd3..37b1caa566 100644 --- a/meson.build +++ b/meson.build @@ -258,11 +258,6 @@ alloc_max = run_command( stack_frame_size = 2048 -# clang without optimization enlarges stack frames in certain corner cases -if cc.get_id() == 'clang' and get_option('optimization') == '0' - stack_frame_size = 4096 -endif - # sanitizer instrumentation may enlarge stack frames if get_option('b_sanitize') != 'none' stack_frame_size = 32768 -- 2.49.0
Michal Privoznik via Devel wrote:
From: Michal Privoznik <mprivozn@redhat.com>
After previous cleanups, all functions have their stack smaller than 2048 bytes and thus the workaround is no longer needed.
Thanks, now that works for me too. Reviewed-by: Roman Bogorodskiy <bogorodskiy@gmail.com>
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- meson.build | 5 ----- 1 file changed, 5 deletions(-)
diff --git a/meson.build b/meson.build index bf4a245dd3..37b1caa566 100644 --- a/meson.build +++ b/meson.build @@ -258,11 +258,6 @@ alloc_max = run_command(
stack_frame_size = 2048
-# clang without optimization enlarges stack frames in certain corner cases -if cc.get_id() == 'clang' and get_option('optimization') == '0' - stack_frame_size = 4096 -endif - # sanitizer instrumentation may enlarge stack frames if get_option('b_sanitize') != 'none' stack_frame_size = 32768 -- 2.49.0
On a Friday in 2025, Michal Privoznik via Devel wrote:
Inspired by Roman's patch:
https://lists.libvirt.org/archives/list/devel@lists.libvirt.org/thread/FNAFO...
There are two functions where clang produces stack greater than 2048 bytes (our current limit - see @stack_frame_size in meson.build):
1) doRemoteOpen() 2) vboxSnapshotRedefine()
It took me a while to realize why a function with a dozen variables requires ~2300 bytes of stack, until the usual suspect turned in - glib.
Anyway, green pipeline:
https://gitlab.com/MichalPrivoznik/libvirt/-/pipelines/1752604895
and I've verified this compiles cleanly on my FreeBSD-14.1-RELEASE-p4 VM.
Michal Prívozník (8): remote_driver: Move URI arg extraction into a separate function doRemoteOpen: Move RPC talk to a separate function doRemoteOpen: Drop needless typecast of @transport vbox: Move parts of vboxSnapshotRedefine() into a separate function vbox: Move parts of vboxSnapshotRedefine() into a separate function vbox: Move parts of vboxSnapshotRedefine() into a separate function vbox: Move parts of vboxSnapshotRedefine() into a separate function meson: Drop workaround for -Wframe-larger-than and clang
meson.build | 5 - src/remote/remote_driver.c | 182 ++++--- src/vbox/vbox_common.c | 946 ++++++++++++++++++++----------------- 3 files changed, 636 insertions(+), 497 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
participants (3)
-
Ján Tomko -
Michal Privoznik -
Roman Bogorodskiy