[libvirt] [PATCHv4 0/5] Handling of undefine and redefine snapshots with VirtualBox 4.2
Hi, This is a serie of patches in order to support undefining and redefining snapshots with VirtualBox 4.2. The serie of patches is rather big, and adds among other things some utility functions unrelated to VirtualBox in patches 1 & 2. The code review could be done in several parts: e.g. patches 1 & 2 separately to validate the utility functions. The VirtualBox API provides only high level operations to manipulate snapshots, so it not possible to support flags like VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE and VIR_DOMAIN_SNAPSHOT_DELETE_METADATA_ONLY with only API calls. Following an IRC talk with Eric Blake, the decision was taken to emulate these behaviours by manipulating directly the .vbox XML files. The first two patches are some util methods for handling uuid and strings that will be used after. The third patch brings more details in the snapshot XML returned by libvirt. We will need those modifications in order to redefine the snapshots. The fourth patch brings the support of the VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE and VIR_DOMAIN_SNAPSHOT_CREATE_CURRENT flags in virDomainSnapshotCreateXML. The fifth and last patch brings the support of the VIR_DOMAIN_SNAPSHOT_DELETE_METADATA_ONLY flag in virDomainSnapshotDelete. The patches are only tested with Virtualbox 4.2 but the code is compliant with Virtualbox 4.3 API. Regards, Manuel VIVES V4: * The code is compliant with Virtualbox 4.3 API * Some minor modifications in order to satisfy "make syntax-check" V3: * Use of STREQ_NULLABLE instead of STREQ in one case * Fix the method for finding uuids according to Ján Tomko review V2: * Fix a licence problem with the method for string replacement Manuel VIVES (5): viruuid.h/c: Util method for finding uuid patterns in some strings virstring.h/c: Util method for making some find and replace in strings vbox_tmpl.c: Better XML description for snapshots vbox_tmpl.c: Patch for redefining snapshots vbox_tmpl.c: Add methods for undefining snapshots po/POTFILES.in | 1 + src/conf/domain_conf.c | 2 +- src/libvirt_private.syms | 2 + src/util/virstring.c | 48 ++ src/util/virstring.h | 2 + src/util/viruuid.c | 81 ++ src/util/viruuid.h | 1 + src/vbox/vbox_tmpl.c | 1854 +++++++++++++++++++++++++++++++++++++++++++--- 8 files changed, 1879 insertions(+), 112 deletions(-) -- 1.7.10.4
--- po/POTFILES.in | 1 + src/libvirt_private.syms | 1 + src/util/viruuid.c | 81 ++++++++++++++++++++++++++++++++++++++++++++++ src/util/viruuid.h | 1 + 4 files changed, 84 insertions(+) diff --git a/po/POTFILES.in b/po/POTFILES.in index 15afdec..451a6fc 100644 --- a/po/POTFILES.in +++ b/po/POTFILES.in @@ -196,6 +196,7 @@ src/util/virtypedparam.c src/util/viruri.c src/util/virusb.c src/util/virutil.c +src/util/viruuid.c src/util/virxml.c src/vbox/vbox_MSCOMGlue.c src/vbox/vbox_XPCOMCGlue.c diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index a705c56..99cc32a 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1915,6 +1915,7 @@ virValidateWWN; # util/viruuid.h virGetHostUUID; +virSearchUuid; virSetHostUUIDStr; virUUIDFormat; virUUIDGenerate; diff --git a/src/util/viruuid.c b/src/util/viruuid.c index c5fa9a8..2cda4ac 100644 --- a/src/util/viruuid.c +++ b/src/util/viruuid.c @@ -34,6 +34,7 @@ #include <sys/stat.h> #include <time.h> #include <unistd.h> +#include <regex.h> #include "c-ctype.h" #include "internal.h" @@ -43,11 +44,14 @@ #include "viralloc.h" #include "virfile.h" #include "virrandom.h" +#include "virstring.h" #ifndef ENODATA # define ENODATA EIO #endif +#define VIR_FROM_THIS VIR_FROM_NONE + static unsigned char host_uuid[VIR_UUID_BUFLEN]; static int @@ -333,3 +337,80 @@ int virGetHostUUID(unsigned char *uuid) return ret; } + + +/** + * virSearchUuid: + * Allows you to get the nth occurrence of a substring in sourceString which matches an uuid pattern. + * If there is no substring, ret is not modified. + * return -1 on error, 0 if not found and 1 if found. + * + * @sourceString: String to parse + * @occurrence: We will return the nth occurrence of uuid in substring, if equals to 0 (or negative), will return the first occurence + * @ret: nth occurrence substring matching an uuid pattern + * @code + char *source = "6853a496-1c10-472e-867a-8244937bd6f0 773ab075-4cd7-4fc2-8b6e-21c84e9cb391 bbb3c75c-d60f-43b0-b802-fd56b84a4222 60c04aa1-0375-4654-8d9f-e149d9885273 4548d465-9891-4c34-a184-3b1c34a26aa8"; + char *ret1=NULL; + char *ret2=NULL; + char *ret3=NULL; + char *ret4=NULL; + virSearchUuid(source, 4,&ret1); //ret1 = "60c04aa1-0375-4654-8d9f-e149d9885273" + virSearchUuid(source, 0,&ret2); //ret2 = "6853a496-1c10-472e-867a-8244937bd6f0" + virSearchUuid(source, 1,&ret3); //ret3 = "6853a496-1c10-472e-867a-8244937bd6f0" + virSearchUuid(source, -4,&ret4); //ret4 = "6853a496-1c10-472e-867a-8244937bd6f0" + * @endcode + */ + +int +virSearchUuid(const char *sourceString, int occurrence, char **ret) +{ + unsigned int position = ((occurrence -1) > 0) ? (occurrence -1) : 0; + const char *uuidRegex = "([a-f0-9]{8}-[a-f0-9]{4}-[a-f0-9]{4}-[a-f0-9]{4}-[a-f0-9]{12})"; + regex_t pregUuidBracket; + size_t i = 0; + size_t nmatch = 0; + regmatch_t *pmatch = NULL; + int retCode = 0; + if (regcomp(&pregUuidBracket, uuidRegex, REG_EXTENDED) != 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Error while compiling regular expression")); + return -1; + } + nmatch = pregUuidBracket.re_nsub; + if (VIR_ALLOC_N(pmatch, nmatch) != 0) { + retCode = -1; + goto cleanup; + } + + while (i < (position+1)) { + if (regexec(&pregUuidBracket, sourceString, nmatch, pmatch, 0) == 0) { + regoff_t start = pmatch[0].rm_so; + regoff_t end = pmatch[0].rm_eo; + if (i == position || (position > i && regexec(&pregUuidBracket, &sourceString[end], nmatch, pmatch, 0) != 0)) { + /* We copy only if i == position (so that it is the uuid we're looking for), or position > i AND + * there is no matches left in the rest of the string (this is the case where we + * give a biggest @occurence than the number of matches and we want to return the last + * one) + */ + if (VIR_STRNDUP(*ret, sourceString + start, end - start) < 0) { + retCode = -1; + goto cleanup; + } + retCode = 1; + goto cleanup; + } + + sourceString = &sourceString[end]; + } else { + break; + retCode = 0; + goto cleanup; + } + ++i; + } + +cleanup: + regfree(&pregUuidBracket); + VIR_FREE(pmatch); + return retCode; +} diff --git a/src/util/viruuid.h b/src/util/viruuid.h index bebd338..2ce4fce 100644 --- a/src/util/viruuid.h +++ b/src/util/viruuid.h @@ -40,4 +40,5 @@ int virUUIDParse(const char *uuidstr, const char *virUUIDFormat(const unsigned char *uuid, char *uuidstr) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); +int virSearchUuid(const char *sourceString, int occurrence, char **ret); #endif /* __VIR_UUID_H__ */ -- 1.7.10.4
On 11/25/2013 07:23 PM, Manuel VIVES wrote:
--- po/POTFILES.in | 1 + src/libvirt_private.syms | 1 + src/util/viruuid.c | 81 ++++++++++++++++++++++++++++++++++++++++++++++ src/util/viruuid.h | 1 + 4 files changed, 84 insertions(+)
diff --git a/po/POTFILES.in b/po/POTFILES.in index 15afdec..451a6fc 100644 --- a/po/POTFILES.in +++ b/po/POTFILES.in @@ -196,6 +196,7 @@ src/util/virtypedparam.c src/util/viruri.c src/util/virusb.c src/util/virutil.c +src/util/viruuid.c src/util/virxml.c src/vbox/vbox_MSCOMGlue.c src/vbox/vbox_XPCOMCGlue.c diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index a705c56..99cc32a 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1915,6 +1915,7 @@ virValidateWWN;
# util/viruuid.h virGetHostUUID; +virSearchUuid; virSetHostUUIDStr; virUUIDFormat; virUUIDGenerate; diff --git a/src/util/viruuid.c b/src/util/viruuid.c index c5fa9a8..2cda4ac 100644 --- a/src/util/viruuid.c +++ b/src/util/viruuid.c @@ -34,6 +34,7 @@ #include <sys/stat.h> #include <time.h> #include <unistd.h> +#include <regex.h>
#include "c-ctype.h" #include "internal.h" @@ -43,11 +44,14 @@ #include "viralloc.h" #include "virfile.h" #include "virrandom.h" +#include "virstring.h"
#ifndef ENODATA # define ENODATA EIO #endif
+#define VIR_FROM_THIS VIR_FROM_NONE + static unsigned char host_uuid[VIR_UUID_BUFLEN];
static int @@ -333,3 +337,80 @@ int virGetHostUUID(unsigned char *uuid)
return ret; } + + +/** + * virSearchUuid: + * Allows you to get the nth occurrence of a substring in sourceString which matches an uuid pattern. + * If there is no substring, ret is not modified. + * return -1 on error, 0 if not found and 1 if found.
Hah! I accidentally started reviewing an earlier version of this patch, and lack of a way to indicate error to the caller was one of my criticisms :-)
+ * + * @sourceString: String to parse + * @occurrence: We will return the nth occurrence of uuid in substring, if equals to 0 (or negative), will return the first occurence + * @ret: nth occurrence substring matching an uuid pattern + * @code + char *source = "6853a496-1c10-472e-867a-8244937bd6f0 773ab075-4cd7-4fc2-8b6e-21c84e9cb391 bbb3c75c-d60f-43b0-b802-fd56b84a4222 60c04aa1-0375-4654-8d9f-e149d9885273 4548d465-9891-4c34-a184-3b1c34a26aa8"; + char *ret1=NULL; + char *ret2=NULL; + char *ret3=NULL; + char *ret4=NULL; + virSearchUuid(source, 4,&ret1); //ret1 = "60c04aa1-0375-4654-8d9f-e149d9885273" + virSearchUuid(source, 0,&ret2); //ret2 = "6853a496-1c10-472e-867a-8244937bd6f0" + virSearchUuid(source, 1,&ret3); //ret3 = "6853a496-1c10-472e-867a-8244937bd6f0" + virSearchUuid(source, -4,&ret4); //ret4 = "6853a496-1c10-472e-867a-8244937bd6f0"
What is the use of having occurrence == -n, 0, or 1 yield the same result? It makes more sense to define the function as "return occurrence 'n' (starting from 0) of a sub-string that matches the pattern for a uuid". Documenting is then much simpler, and it's easier to know the "best" way to get the first occurrence (since there is only one way to do it). This seems like a very specific use case for a more general function. How about making a virSearchRegex() function (or maybe some other name) which took the regex as another arg? That would make it more likely that the code would be reused.
+ * @endcode + */ + +int +virSearchUuid(const char *sourceString, int occurrence, char **ret) +{ + unsigned int position = ((occurrence -1) > 0) ? (occurrence -1) : 0; + const char *uuidRegex = "([a-f0-9]{8}-[a-f0-9]{4}-[a-f0-9]{4}-[a-f0-9]{4}-[a-f0-9]{12})"; + regex_t pregUuidBracket; + size_t i = 0; + size_t nmatch = 0; + regmatch_t *pmatch = NULL; + int retCode = 0; + if (regcomp(&pregUuidBracket, uuidRegex, REG_EXTENDED) != 0) {
You're missing a blank line after the end of the data declarations. Also, it's more common in libvirt to call the variable containing the eventual function return value "ret" rather than "retCode", and initialize it to -1. Then you just unconditionally set it to 0 just before a successful return (or in this case, set it to 0 for the "not found" case or 1 for the "found" case), but don't have to do anything to it for all the failure cases.
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Error while compiling regular expression"));
You may as well collect the return value from regcomp and include it in the error message. Even if just as %d, it may help to debug in the unlikely case an error is ever encountered.
+ return -1; + } + nmatch = pregUuidBracket.re_nsub; + if (VIR_ALLOC_N(pmatch, nmatch) != 0) {
For consistency, we try to check the return from all libvirt functions with "< 0" instead of "!= 0", so: if (VIR_ALLOC_N(pmatch, nmatch) < 0) ...
+ retCode = -1; + goto cleanup; + }
(here, for example, if you had initialized retCode (ret) to -1, you could just do "goto cleanup" and wouldn't even need the brackets.
+ + while (i < (position+1)) { + if (regexec(&pregUuidBracket, sourceString, nmatch, pmatch, 0) == 0) { + regoff_t start = pmatch[0].rm_so; + regoff_t end = pmatch[0].rm_eo; + if (i == position || (position > i && regexec(&pregUuidBracket, &sourceString[end], nmatch, pmatch, 0) != 0)) { + /* We copy only if i == position (so that it is the uuid we're looking for), or position > i AND + * there is no matches left in the rest of the string (this is the case where we + * give a biggest @occurence than the number of matches and we want to return the last + * one) + */
Please reformat the comments to fit within 80 colums (this same comment applies to the comments at the top of the function, as well as for code, whenever practically possible).
+ if (VIR_STRNDUP(*ret, sourceString + start, end - start) < 0) { + retCode = -1; + goto cleanup; + } + retCode = 1; + goto cleanup; + } + + sourceString = &sourceString[end]; + } else { + break; + retCode = 0; + goto cleanup; + } + ++i; + } + +cleanup: + regfree(&pregUuidBracket); + VIR_FREE(pmatch); + return retCode; +} diff --git a/src/util/viruuid.h b/src/util/viruuid.h index bebd338..2ce4fce 100644 --- a/src/util/viruuid.h +++ b/src/util/viruuid.h @@ -40,4 +40,5 @@ int virUUIDParse(const char *uuidstr, const char *virUUIDFormat(const unsigned char *uuid, char *uuidstr) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
+int virSearchUuid(const char *sourceString, int occurrence, char **ret); #endif /* __VIR_UUID_H__ */
On 11/25/2013 07:23 PM, Manuel VIVES wrote:
---
po/POTFILES.in | 1 + src/libvirt_private.syms | 1 + src/util/viruuid.c | 81 ++++++++++++++++++++++++++++++++++++++++++++++ src/util/viruuid.h | 1 + 4 files changed, 84 insertions(+)
diff --git a/po/POTFILES.in b/po/POTFILES.in index 15afdec..451a6fc 100644 --- a/po/POTFILES.in +++ b/po/POTFILES.in @@ -196,6 +196,7 @@ src/util/virtypedparam.c
src/util/viruri.c src/util/virusb.c src/util/virutil.c
+src/util/viruuid.c
src/util/virxml.c src/vbox/vbox_MSCOMGlue.c src/vbox/vbox_XPCOMCGlue.c
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index a705c56..99cc32a 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1915,6 +1915,7 @@ virValidateWWN;
# util/viruuid.h virGetHostUUID;
+virSearchUuid;
virSetHostUUIDStr; virUUIDFormat; virUUIDGenerate;
diff --git a/src/util/viruuid.c b/src/util/viruuid.c index c5fa9a8..2cda4ac 100644 --- a/src/util/viruuid.c +++ b/src/util/viruuid.c @@ -34,6 +34,7 @@
#include <sys/stat.h> #include <time.h> #include <unistd.h>
+#include <regex.h>
#include "c-ctype.h" #include "internal.h"
@@ -43,11 +44,14 @@
#include "viralloc.h" #include "virfile.h" #include "virrandom.h"
+#include "virstring.h"
#ifndef ENODATA # define ENODATA EIO #endif
+#define VIR_FROM_THIS VIR_FROM_NONE +
static unsigned char host_uuid[VIR_UUID_BUFLEN];
static int
@@ -333,3 +337,80 @@ int virGetHostUUID(unsigned char *uuid)
return ret;
}
+ + +/** + * virSearchUuid: + * Allows you to get the nth occurrence of a substring in sourceString which matches an uuid pattern. + * If there is no substring, ret is not modified. + * return -1 on error, 0 if not found and 1 if found.
Hah! I accidentally started reviewing an earlier version of this patch, and lack of a way to indicate error to the caller was one of my criticisms :-)
+ * + * @sourceString: String to parse + * @occurrence: We will return the nth occurrence of uuid in substring, if equals to 0 (or negative), will return the first occurence + * @ret: nth occurrence substring matching an uuid pattern + * @code + char *source = "6853a496-1c10-472e-867a-8244937bd6f0 773ab075-4cd7-4fc2-8b6e-21c84e9cb391 bbb3c75c-d60f-43b0-b802-fd56b84a4222 60c04aa1-0375-4654-8d9f-e149d9885273 4548d465-9891-4c34-a184-3b1c34a26aa8"; + char *ret1=NULL; + char *ret2=NULL; + char *ret3=NULL; + char *ret4=NULL; + virSearchUuid(source, 4,&ret1); //ret1 = "60c04aa1-0375-4654-8d9f-e149d9885273" + virSearchUuid(source, 0,&ret2); //ret2 = "6853a496-1c10-472e-867a-8244937bd6f0" + virSearchUuid(source, 1,&ret3); //ret3 = "6853a496-1c10-472e-867a-8244937bd6f0" + virSearchUuid(source, -4,&ret4); //ret4 = "6853a496-1c10-472e-867a-8244937bd6f0"
What is the use of having occurrence == -n, 0, or 1 yield the same result? It makes more sense to define the function as "return occurrence 'n' (starting from 0) of a sub-string that matches the pattern for a uuid". Documenting is then much simpler, and it's easier to know the "best" way to get the first occurrence (since there is only one way to do it).
This seems like a very specific use case for a more general function. How about making a virSearchRegex() function (or maybe some other name) which took the regex as another arg? That would make it more likely that the code would be reused.
I have modified this method, so now it takes a regex as argument and I have renamed it to 'virSearchRegex'. Should I move it in another file (it is still in viruuid.c/h)?
+ * @endcode + */ + +int +virSearchUuid(const char *sourceString, int occurrence, char **ret) +{ + unsigned int position = ((occurrence -1) > 0) ? (occurrence -1) : 0; + const char *uuidRegex = "([a-f0-9]{8}-[a-f0-9]{4}-[a-f0-9]{4}-[a-f0-9]{4}-[a-f0-9]{12})"; + regex_t pregUuidBracket; + size_t i = 0; + size_t nmatch = 0; + regmatch_t *pmatch = NULL; + int retCode = 0; + if (regcomp(&pregUuidBracket, uuidRegex, REG_EXTENDED) != 0) {
You're missing a blank line after the end of the data declarations. Also, it's more common in libvirt to call the variable containing the eventual function return value "ret" rather than "retCode", and initialize it to -1. Then you just unconditionally set it to 0 just before a successful return (or in this case, set it to 0 for the "not found" case or 1 for the "found" case), but don't have to do anything to it for all the failure cases.
I use retCode because I already have a parameter named ret which will contains the string matching, perhaps I should rename my parameter and use ret instead of retCode.
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Error while compiling regular expression"));
You may as well collect the return value from regcomp and include it in the error message. Even if just as %d, it may help to debug in the unlikely case an error is ever encountered.
+ return -1; + } + nmatch = pregUuidBracket.re_nsub; + if (VIR_ALLOC_N(pmatch, nmatch) != 0) {
For consistency, we try to check the return from all libvirt functions with "< 0" instead of "!= 0", so:
if (VIR_ALLOC_N(pmatch, nmatch) < 0) ...
+ retCode = -1; + goto cleanup; + }
(here, for example, if you had initialized retCode (ret) to -1, you could just do "goto cleanup" and wouldn't even need the brackets.
+ + while (i < (position+1)) { + if (regexec(&pregUuidBracket, sourceString, nmatch, pmatch, 0) == 0) { + regoff_t start = pmatch[0].rm_so; + regoff_t end = pmatch[0].rm_eo; + if (i == position || (position > i && regexec(&pregUuidBracket, &sourceString[end], nmatch, pmatch, 0) != 0)) { + /* We copy only if i == position (so that it is the uuid we're looking for), or position > i AND + * there is no matches left in the rest of the string (this is the case where we + * give a biggest @occurence than the number of matches and we want to return the last + * one) + */
Please reformat the comments to fit within 80 colums (this same comment applies to the comments at the top of the function, as well as for code, whenever practically possible).
+ if (VIR_STRNDUP(*ret, sourceString + start, end - start) < 0) { + retCode = -1; + goto cleanup; + } + retCode = 1; + goto cleanup; + } + + sourceString = &sourceString[end]; + } else { + break; + retCode = 0; + goto cleanup; + } + ++i; + } + +cleanup: + regfree(&pregUuidBracket); + VIR_FREE(pmatch); + return retCode; +} diff --git a/src/util/viruuid.h b/src/util/viruuid.h index bebd338..2ce4fce 100644 --- a/src/util/viruuid.h +++ b/src/util/viruuid.h @@ -40,4 +40,5 @@ int virUUIDParse(const char *uuidstr,
const char *virUUIDFormat(const unsigned char *uuid,
char *uuidstr) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
+int virSearchUuid(const char *sourceString, int occurrence, char **ret);
#endif /* __VIR_UUID_H__ */
On 12/23/2013 05:47 PM, Manuel VIVES wrote:
On 11/25/2013 07:23 PM, Manuel VIVES wrote:
+ * + * @sourceString: String to parse + * @occurrence: We will return the nth occurrence of uuid in substring, if equals to 0 (or negative), will return the first occurence + * @ret: nth occurrence substring matching an uuid pattern + * @code + char *source = "6853a496-1c10-472e-867a-8244937bd6f0 773ab075-4cd7-4fc2-8b6e-21c84e9cb391 bbb3c75c-d60f-43b0-b802-fd56b84a4222 60c04aa1-0375-4654-8d9f-e149d9885273 4548d465-9891-4c34-a184-3b1c34a26aa8"; + char *ret1=NULL; + char *ret2=NULL; + char *ret3=NULL; + char *ret4=NULL; + virSearchUuid(source, 4,&ret1); //ret1 = "60c04aa1-0375-4654-8d9f-e149d9885273" + virSearchUuid(source, 0,&ret2); //ret2 = "6853a496-1c10-472e-867a-8244937bd6f0" + virSearchUuid(source, 1,&ret3); //ret3 = "6853a496-1c10-472e-867a-8244937bd6f0" + virSearchUuid(source, -4,&ret4); //ret4 = "6853a496-1c10-472e-867a-8244937bd6f0" What is the use of having occurrence == -n, 0, or 1 yield the same result? It makes more sense to define the function as "return occurrence 'n' (starting from 0) of a sub-string that matches the pattern for a uuid". Documenting is then much simpler, and it's easier to know the "best" way to get the first occurrence (since there is only one way to do it).
This seems like a very specific use case for a more general function. How about making a virSearchRegex() function (or maybe some other name) which took the regex as another arg? That would make it more likely that the code would be reused.
I have modified this method, so now it takes a regex as argument and I have renamed it to 'virSearchRegex'. Should I move it in another file (it is still in viruuid.c/h)?
virstring.[ch] is probably the best place for it.
+ * @endcode + */ + +int +virSearchUuid(const char *sourceString, int occurrence, char **ret) +{ + unsigned int position = ((occurrence -1) > 0) ? (occurrence -1) : 0; + const char *uuidRegex = "([a-f0-9]{8}-[a-f0-9]{4}-[a-f0-9]{4}-[a-f0-9]{4}-[a-f0-9]{12})"; + regex_t pregUuidBracket; + size_t i = 0; + size_t nmatch = 0; + regmatch_t *pmatch = NULL; + int retCode = 0; + if (regcomp(&pregUuidBracket, uuidRegex, REG_EXTENDED) != 0) { You're missing a blank line after the end of the data declarations. Also, it's more common in libvirt to call the variable containing the eventual function return value "ret" rather than "retCode", and initialize it to -1. Then you just unconditionally set it to 0 just before a successful return (or in this case, set it to 0 for the "not found" case or 1 for the "found" case), but don't have to do anything to it for all the failure cases.
I use retCode because I already have a parameter named ret which will contains the string matching, perhaps I should rename my parameter and use ret instead of retCode.
Yes, that would be more consistent, and consistency helps to avoid future mistakes.
--- src/libvirt_private.syms | 1 + src/util/virstring.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++ src/util/virstring.h | 2 ++ 3 files changed, 51 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 99cc32a..b761fb6 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1750,6 +1750,7 @@ virStringListLength; virStringSplit; virStrncpy; virStrndup; +virStrReplace; virStrToDouble; virStrToLong_i; virStrToLong_l; diff --git a/src/util/virstring.c b/src/util/virstring.c index d11db5c..a30a4ef 100644 --- a/src/util/virstring.c +++ b/src/util/virstring.c @@ -616,3 +616,51 @@ size_t virStringListLength(char **strings) return i; } + +/* + virStrReplace(haystack, oldneedle, newneedle) -- + Search haystack and replace all occurences of oldneedle with newneedle. + Return a string with all the replacements in case of success, NULL in case + of failure +*/ +char * +virStrReplace(char *haystack, + const char *oldneedle, const char *newneedle) +{ + char *ret; + size_t i, count = 0; + size_t newneedle_len = strlen(newneedle); + size_t oldneedle_len = strlen(oldneedle); + size_t totalLength = 0; + if (strlen(oldneedle) == 0) { + ignore_value(VIR_STRDUP(ret, haystack)); + goto cleanup; + } + + for (i = 0; haystack[i] != '\0'; i++) { + if (strstr(&haystack[i], oldneedle) == &haystack[i]) { + count++; + i += oldneedle_len - 1; + } + } + if (VIR_ALLOC_N(ret, (i + count * (newneedle_len - oldneedle_len))) < 0) { + ret = NULL; + goto cleanup; + } + totalLength = sizeof(char *)*(i + count * (newneedle_len - oldneedle_len)); + i = 0; + while (*haystack) { + if (strstr(haystack, oldneedle) == haystack) { + if (virStrcpy(&ret[i], newneedle, totalLength) == NULL) { + ret = NULL; + goto cleanup; + } + i += newneedle_len; + haystack += oldneedle_len; + } else + ret[i++] = *haystack++; + } + ret[i] = '\0'; +cleanup: + return ret; +} diff --git a/src/util/virstring.h b/src/util/virstring.h index b390150..90522bd 100644 --- a/src/util/virstring.h +++ b/src/util/virstring.h @@ -223,4 +223,6 @@ size_t virStringListLength(char **strings); virAsprintfInternal(false, 0, NULL, NULL, 0, \ strp, __VA_ARGS__) +char * virStrReplace(char *haystack, const char *oldneedle, const char *newneedle); + #endif /* __VIR_STRING_H__ */ -- 1.7.10.4
On 11/25/2013 07:23 PM, Manuel VIVES wrote:
--- src/libvirt_private.syms | 1 + src/util/virstring.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++ src/util/virstring.h | 2 ++ 3 files changed, 51 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 99cc32a..b761fb6 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1750,6 +1750,7 @@ virStringListLength; virStringSplit; virStrncpy; virStrndup; +virStrReplace; virStrToDouble; virStrToLong_i; virStrToLong_l; diff --git a/src/util/virstring.c b/src/util/virstring.c index d11db5c..a30a4ef 100644 --- a/src/util/virstring.c +++ b/src/util/virstring.c @@ -616,3 +616,51 @@ size_t virStringListLength(char **strings)
return i; } + +/* + virStrReplace(haystack, oldneedle, newneedle) -- + Search haystack and replace all occurences of oldneedle with newneedle. + Return a string with all the replacements in case of success, NULL in case + of failure +*/ +char * +virStrReplace(char *haystack, + const char *oldneedle, const char *newneedle) +{ + char *ret; + size_t i, count = 0; + size_t newneedle_len = strlen(newneedle); + size_t oldneedle_len = strlen(oldneedle); + size_t totalLength = 0; + if (strlen(oldneedle) == 0) {
You can just check oldneedle_len here since it already contains strlen(oldneedle). Also, should this really be a NOP? It makes no sense to ask to replace all occurrences of "" with anything (kind of like dividing by 0), so I think it should rather be an error (likewise, do we need to check for !oldneedle or !newneedle ? Or are the callers controlled enough that we can just put that requirement on them? If the latter, then the declaration in the .h file needs to have "ATTRIBUTE_NONNULL(2), ATTRIBUTE_NONNULL(3) (and we probably should have ATTRIBUTE_NONNULL(1) in any case, unless there is a danger a caller could send a null haystack)
+ ignore_value(VIR_STRDUP(ret, haystack)); + goto cleanup; + } + + for (i = 0; haystack[i] != '\0'; i++) { + if (strstr(&haystack[i], oldneedle) == &haystack[i]) {
This just makes no sense. strstr is used to search for a substring within a string. Here you are searching for a substring, but only counting it if the match is at the beginning of the string. If you're going to do that, why not just use memcmp() instead and save all the extra scanning? Or better yet, just let strstr() do its work and set "i = [return from strstr] + oldneedle_len" when you get a match.
+ count++; + i += oldneedle_len - 1; + } + }
Okay, so at this point i == strlen(haystack)...
+ if (VIR_ALLOC_N(ret, (i + count * (newneedle_len - oldneedle_len))) < 0) {
You need to allocate one extra byte for the terminating \0.
+ ret = NULL;
No need to set ret = NULL - that is done by VIR_ALLOC_N.
+ goto cleanup; + } + totalLength = sizeof(char *)*(i + count * (newneedle_len - oldneedle_len));
This is incorrect and potentially dangerous - "sizeof(char*) is the size of a char* (usually 8 bytes), NOT the size of a char (1 byte), so you're telling virStrcpy() that there are 8 times as many bytes available as there actually are.
+ i = 0; + while (*haystack) { + if (strstr(haystack, oldneedle) == haystack) {
Again, if you're only going to count a match when it is exactly at the start of the string, you should just use memcmp instead of strstr. Or alternately, make the algorithm more intelligent so that it takes advantage of any platform-specific optimizations that may be hidden in strstr.
+ if (virStrcpy(&ret[i], newneedle, totalLength) == NULL) {
Even if you had set the value of totalLength properly above (you didn't), this is also incorrect, because you haven't accounted for the fact that you're copying into the middle of the string, not the start.
+ ret = NULL;
You just leaked the memory at ret. Once you've allocated memory to it, if you're going to return failure you need to do "VIR_FREE(ret) instead (which will free the memory and set it to NULL).
+ goto cleanup; + } + i += newneedle_len; + haystack += oldneedle_len; + } else + ret[i++] = *haystack++; + }
I'm not going to try and verify correctness of the remainder of this function, because it is broken and needs reworking anyway.
+ ret[i] = '\0'; +cleanup: + return ret; +} diff --git a/src/util/virstring.h b/src/util/virstring.h index b390150..90522bd 100644 --- a/src/util/virstring.h +++ b/src/util/virstring.h @@ -223,4 +223,6 @@ size_t virStringListLength(char **strings); virAsprintfInternal(false, 0, NULL, NULL, 0, \ strp, __VA_ARGS__)
+char * virStrReplace(char *haystack, const char *oldneedle, const char *newneedle); + #endif /* __VIR_STRING_H__ */
On 11/25/2013 07:23 PM, Manuel VIVES wrote:
---
src/libvirt_private.syms | 1 + src/util/virstring.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++ src/util/virstring.h | 2 ++ 3 files changed, 51 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 99cc32a..b761fb6 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1750,6 +1750,7 @@ virStringListLength;
virStringSplit; virStrncpy; virStrndup;
+virStrReplace;
virStrToDouble; virStrToLong_i; virStrToLong_l;
diff --git a/src/util/virstring.c b/src/util/virstring.c index d11db5c..a30a4ef 100644 --- a/src/util/virstring.c +++ b/src/util/virstring.c @@ -616,3 +616,51 @@ size_t virStringListLength(char **strings)
return i;
}
+ +/* + virStrReplace(haystack, oldneedle, newneedle) -- + Search haystack and replace all occurences of oldneedle with newneedle. + Return a string with all the replacements in case of success, NULL in case + of failure +*/ +char * +virStrReplace(char *haystack, + const char *oldneedle, const char *newneedle) +{ + char *ret; + size_t i, count = 0; + size_t newneedle_len = strlen(newneedle); + size_t oldneedle_len = strlen(oldneedle); + size_t totalLength = 0; + if (strlen(oldneedle) == 0) {
You can just check oldneedle_len here since it already contains strlen(oldneedle).
Also, should this really be a NOP? It makes no sense to ask to replace all occurrences of "" with anything (kind of like dividing by 0), so I think it should rather be an error (likewise, do we need to check for !oldneedle or !newneedle ? Or are the callers controlled enough that we can just put that requirement on them? If the latter, then the declaration in the .h file needs to have "ATTRIBUTE_NONNULL(2), ATTRIBUTE_NONNULL(3) (and we probably should have ATTRIBUTE_NONNULL(1) in any case, unless there is a danger a caller could send a null haystack)
I'm going to modify my patch in order to return an error if oldneedle is empty. I also looked at ATTRIBUTE_NONNULL and I did't really understand the purpose so if someone could explain it ;) But maybe I should also include the !oldneedle, the !newneedle and !haystack tests inside the method, what do you think?
+ ignore_value(VIR_STRDUP(ret, haystack)); + goto cleanup; + } + + for (i = 0; haystack[i] != '\0'; i++) { + if (strstr(&haystack[i], oldneedle) == &haystack[i]) {
This just makes no sense. strstr is used to search for a substring within a string. Here you are searching for a substring, but only counting it if the match is at the beginning of the string. If you're going to do that, why not just use memcmp() instead and save all the extra scanning?
Or better yet, just let strstr() do its work and set "i = [return from strstr] + oldneedle_len" when you get a match.
+ count++; + i += oldneedle_len - 1; + } + }
Okay, so at this point i == strlen(haystack)...
+ if (VIR_ALLOC_N(ret, (i + count * (newneedle_len - oldneedle_len))) < 0) {
You need to allocate one extra byte for the terminating \0.
+ ret = NULL;
No need to set ret = NULL - that is done by VIR_ALLOC_N.
+ goto cleanup; + } + totalLength = sizeof(char *)*(i + count * (newneedle_len - oldneedle_len));
This is incorrect and potentially dangerous - "sizeof(char*) is the size of a char* (usually 8 bytes), NOT the size of a char (1 byte), so you're telling virStrcpy() that there are 8 times as many bytes available as there actually are.
+ i = 0; + while (*haystack) { + if (strstr(haystack, oldneedle) == haystack) {
Again, if you're only going to count a match when it is exactly at the start of the string, you should just use memcmp instead of strstr. Or alternately, make the algorithm more intelligent so that it takes advantage of any platform-specific optimizations that may be hidden in strstr.
+ if (virStrcpy(&ret[i], newneedle, totalLength) == NULL) {
Even if you had set the value of totalLength properly above (you didn't), this is also incorrect, because you haven't accounted for the fact that you're copying into the middle of the string, not the start.
+ ret = NULL;
You just leaked the memory at ret. Once you've allocated memory to it, if you're going to return failure you need to do "VIR_FREE(ret) instead (which will free the memory and set it to NULL).
+ goto cleanup; + } + i += newneedle_len; + haystack += oldneedle_len; + } else + ret[i++] = *haystack++; + }
I'm not going to try and verify correctness of the remainder of this function, because it is broken and needs reworking anyway.
+ ret[i] = '\0'; +cleanup: + return ret; +} diff --git a/src/util/virstring.h b/src/util/virstring.h index b390150..90522bd 100644 --- a/src/util/virstring.h +++ b/src/util/virstring.h @@ -223,4 +223,6 @@ size_t virStringListLength(char **strings);
virAsprintfInternal(false, 0, NULL, NULL, 0, \
strp, __VA_ARGS__)
+char * virStrReplace(char *haystack, const char *oldneedle, const char *newneedle); +
#endif /* __VIR_STRING_H__ */
On 12/23/2013 05:47 PM, Manuel VIVES wrote:
On 11/25/2013 07:23 PM, Manuel VIVES wrote:
---
src/libvirt_private.syms | 1 + src/util/virstring.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++ src/util/virstring.h | 2 ++ 3 files changed, 51 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 99cc32a..b761fb6 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1750,6 +1750,7 @@ virStringListLength;
virStringSplit; virStrncpy; virStrndup;
+virStrReplace;
virStrToDouble; virStrToLong_i; virStrToLong_l;
diff --git a/src/util/virstring.c b/src/util/virstring.c index d11db5c..a30a4ef 100644 --- a/src/util/virstring.c +++ b/src/util/virstring.c @@ -616,3 +616,51 @@ size_t virStringListLength(char **strings)
return i;
}
+ +/* + virStrReplace(haystack, oldneedle, newneedle) -- + Search haystack and replace all occurences of oldneedle with newneedle. + Return a string with all the replacements in case of success, NULL in case + of failure +*/ +char * +virStrReplace(char *haystack, + const char *oldneedle, const char *newneedle) +{ + char *ret; + size_t i, count = 0; + size_t newneedle_len = strlen(newneedle); + size_t oldneedle_len = strlen(oldneedle); + size_t totalLength = 0; + if (strlen(oldneedle) == 0) { You can just check oldneedle_len here since it already contains strlen(oldneedle).
Also, should this really be a NOP? It makes no sense to ask to replace all occurrences of "" with anything (kind of like dividing by 0), so I think it should rather be an error (likewise, do we need to check for !oldneedle or !newneedle ? Or are the callers controlled enough that we can just put that requirement on them? If the latter, then the declaration in the .h file needs to have "ATTRIBUTE_NONNULL(2), ATTRIBUTE_NONNULL(3) (and we probably should have ATTRIBUTE_NONNULL(1) in any case, unless there is a danger a caller could send a null haystack)
I'm going to modify my patch in order to return an error if oldneedle is empty. I also looked at ATTRIBUTE_NONNULL and I did't really understand the purpose so if someone could explain it ;)
ATTRIBUTE_NONNULL() is intended as a potential hint to the compiler's optimizer, or to any static analysis tool such as coverity, that the function will never be called with that particular argument being NULL. Whether or not that information is put to any use is up to the compiler/analyzer. In general, if an argument is given a very controlled set of values (e.g. if it is always specified as a constant value, or if all of the callers have already checked that arg for non-NULL) then it can be useful to declare it as ATTRIBUTE_NONNULL.
But maybe I should also include the !oldneedle, the !newneedle and !haystack tests inside the method, what do you think?
The choice is yours, but if you don't check, then you need to document that in the description of the function so that future callers will know to check for non-NULL prior to calling (unless they are already certain).
It will be needed for the futur patches because we will redefine snapshots --- src/conf/domain_conf.c | 2 +- src/vbox/vbox_tmpl.c | 427 ++++++++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 417 insertions(+), 12 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 7b0e3ea..ae20eb5 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -17054,7 +17054,7 @@ virDomainDefFormatInternal(virDomainDefPtr def, if (virDomainChrDefFormat(buf, &console, flags) < 0) goto error; } - if (STREQ(def->os.type, "hvm") && + if (STREQ_NULLABLE(def->os.type, "hvm") && def->nconsoles == 0 && def->nserials > 0) { virDomainChrDef console; diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c index 983a595..e05ed97 100644 --- a/src/vbox/vbox_tmpl.c +++ b/src/vbox/vbox_tmpl.c @@ -38,6 +38,7 @@ #include <sys/types.h> #include <sys/stat.h> #include <fcntl.h> +#include <libxml/xmlwriter.h> #include "internal.h" #include "datatypes.h" @@ -58,6 +59,8 @@ #include "fdstream.h" #include "viruri.h" #include "virstring.h" +#include "virtime.h" +#include "virutil.h" /* This one changes from version to version. */ #if VBOX_API_VERSION == 2002 @@ -273,10 +276,16 @@ static vboxGlobalData *g_pVBoxGlobalData = NULL; #endif /* VBOX_API_VERSION >= 4000 */ +#define reportInternalErrorIfNS_FAILED(message) \ + if (NS_FAILED(rc)) { \ + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _(message)); \ + goto cleanup; \ + } + + static virDomainPtr vboxDomainDefineXML(virConnectPtr conn, const char *xml); static int vboxDomainCreate(virDomainPtr dom); static int vboxDomainUndefineFlags(virDomainPtr dom, unsigned int flags); - static void vboxDriverLock(vboxGlobalData *data) { virMutexLock(&data->lock); } @@ -285,6 +294,12 @@ static void vboxDriverUnlock(vboxGlobalData *data) { virMutexUnlock(&data->lock); } +typedef enum { + VBOX_STORAGE_DELETE_FLAG = 0, +#if VBOX_API_VERSION >= 4002 + VBOX_STORAGE_CLOSE_FLAG = 1, +#endif +} vboxStorageDeleteOrCloseFlags; #if VBOX_API_VERSION == 2002 static void nsIDtoChar(unsigned char *uuid, const nsID *iid) { @@ -5957,7 +5972,8 @@ vboxDomainSnapshotCreateXML(virDomainPtr dom, virCheckFlags(VIR_DOMAIN_SNAPSHOT_CREATE_NO_METADATA, NULL); if (!(def = virDomainSnapshotDefParseString(xmlDesc, data->caps, - data->xmlopt, 0, 0))) + data->xmlopt, -1, VIR_DOMAIN_SNAPSHOT_PARSE_DISKS | + VIR_DOMAIN_SNAPSHOT_PARSE_REDEFINE))) goto cleanup; if (def->ndisks) { @@ -5965,7 +5981,6 @@ vboxDomainSnapshotCreateXML(virDomainPtr dom, _("disk snapshots not supported yet")); goto cleanup; } - vboxIIDFromUUID(&domiid, dom->uuid); rc = VBOX_OBJECT_GET_MACHINE(domiid.value, &machine); if (NS_FAILED(rc)) { @@ -5973,7 +5988,6 @@ vboxDomainSnapshotCreateXML(virDomainPtr dom, _("no domain with matching UUID")); goto cleanup; } - rc = machine->vtbl->GetState(machine, &state); if (NS_FAILED(rc)) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", @@ -6048,6 +6062,344 @@ cleanup: return ret; } +#if VBOX_API_VERSION >=4002 +static +int vboxSnapshotGetReadWriteDisks(virDomainSnapshotDefPtr def, + virDomainSnapshotPtr snapshot) +{ + virDomainPtr dom = snapshot->domain; + VBOX_OBJECT_CHECK(dom->conn, int, -1); + vboxIID domiid = VBOX_IID_INITIALIZER; + IMachine *machine = NULL; + ISnapshot *snap = NULL; + IMachine *snapMachine = NULL; + bool error = false; + vboxArray mediumAttachments = VBOX_ARRAY_INITIALIZER; + PRUint32 maxPortPerInst[StorageBus_Floppy + 1] = {}; + PRUint32 maxSlotPerPort[StorageBus_Floppy + 1] = {}; + int diskCount = 0; + nsresult rc; + vboxIID snapIid = VBOX_IID_INITIALIZER; + char *snapshotUuidStr = NULL; + size_t i = 0; + vboxIIDFromUUID(&domiid, dom->uuid); + rc = VBOX_OBJECT_GET_MACHINE(domiid.value, &machine); + reportInternalErrorIfNS_FAILED("no domain with matching UUID"); + if (!(snap = vboxDomainSnapshotGet(data, dom, machine, snapshot->name))) { + ret = -1; + goto cleanup; + } + rc = snap->vtbl->GetId(snap, &snapIid.value); + reportInternalErrorIfNS_FAILED("Could not get snapshot id"); + + VBOX_UTF16_TO_UTF8(snapIid.value, &snapshotUuidStr); + rc = snap->vtbl->GetMachine(snap, &snapMachine); + reportInternalErrorIfNS_FAILED("could not get machine"); + def->ndisks = 0; + rc = vboxArrayGet(&mediumAttachments, snapMachine, snapMachine->vtbl->GetMediumAttachments); + reportInternalErrorIfNS_FAILED("no medium attachments"); + /* get the number of attachments */ + for (i = 0; i < mediumAttachments.count; i++) { + IMediumAttachment *imediumattach = mediumAttachments.items[i]; + if (imediumattach) { + IMedium *medium = NULL; + + rc = imediumattach->vtbl->GetMedium(imediumattach, &medium); + reportInternalErrorIfNS_FAILED("cannot get medium"); + if (medium) { + def->ndisks++; + VBOX_RELEASE(medium); + } + } + } + /* Allocate mem, if fails return error */ + if (VIR_ALLOC_N(def->disks, def->ndisks) < 0) { + virReportOOMError(); + error = true; + } + if (!error) + error = !vboxGetMaxPortSlotValues(data->vboxObj, maxPortPerInst, maxSlotPerPort); + + /* get the attachment details here */ + for (i = 0; i < mediumAttachments.count && diskCount < def->ndisks && !error; i++) { + IStorageController *storageController = NULL; + PRUnichar *storageControllerName = NULL; + PRUint32 deviceType = DeviceType_Null; + PRUint32 storageBus = StorageBus_Null; + IMedium *disk = NULL; + PRUnichar *childLocUtf16 = NULL; + char *childLocUtf8 = NULL; + PRUint32 deviceInst = 0; + PRInt32 devicePort = 0; + PRInt32 deviceSlot = 0; + vboxArray children = VBOX_ARRAY_INITIALIZER; + vboxArray snapshotIids = VBOX_ARRAY_INITIALIZER; + IMediumAttachment *imediumattach = mediumAttachments.items[i]; + size_t j = 0; + size_t k = 0; + if (!imediumattach) + continue; + rc = imediumattach->vtbl->GetMedium(imediumattach, &disk); + reportInternalErrorIfNS_FAILED("cannot get medium"); + if (!disk) + continue; + rc = imediumattach->vtbl->GetController(imediumattach, &storageControllerName); + reportInternalErrorIfNS_FAILED("cannot get medium"); + if (!storageControllerName) { + VBOX_RELEASE(disk); + continue; + } + rc= vboxArrayGet(&children, disk, disk->vtbl->GetChildren); + reportInternalErrorIfNS_FAILED("cannot get children disk"); + rc = vboxArrayGetWithPtrArg(&snapshotIids, disk, disk->vtbl->GetSnapshotIds, domiid.value); + reportInternalErrorIfNS_FAILED("cannot get snapshot ids"); + for (j = 0; j < children.count; ++j) { + IMedium *child = children.items[j]; + for (k = 0; k < snapshotIids.count; ++k) { + PRUnichar *diskSnapId = snapshotIids.items[k]; + char *diskSnapIdStr = NULL; + VBOX_UTF16_TO_UTF8(diskSnapId, &diskSnapIdStr); + if (STREQ(diskSnapIdStr, snapshotUuidStr)) { + rc = machine->vtbl->GetStorageControllerByName(machine, + storageControllerName, + &storageController); + VBOX_UTF16_FREE(storageControllerName); + if (!storageController) { + VBOX_RELEASE(child); + break; + } + rc = child->vtbl->GetLocation(child, &childLocUtf16); + reportInternalErrorIfNS_FAILED("cannot get disk location"); + VBOX_UTF16_TO_UTF8(childLocUtf16, &childLocUtf8); + VBOX_UTF16_FREE(childLocUtf16); + ignore_value(VIR_STRDUP(def->disks[diskCount].file, childLocUtf8)); + if (!(def->disks[diskCount].file)) { + VBOX_RELEASE(child); + VBOX_RELEASE(storageController); + virReportOOMError(); + error = true; + break; + } + VBOX_UTF8_FREE(childLocUtf8); + + rc = storageController->vtbl->GetBus(storageController, &storageBus); + reportInternalErrorIfNS_FAILED("cannot get storage controller bus"); + rc = imediumattach->vtbl->GetType(imediumattach, &deviceType); + reportInternalErrorIfNS_FAILED("cannot get medium attachment type"); + rc = imediumattach->vtbl->GetPort(imediumattach, &devicePort); + reportInternalErrorIfNS_FAILED("cannot get medium attachchment type"); + rc = imediumattach->vtbl->GetDevice(imediumattach, &deviceSlot); + reportInternalErrorIfNS_FAILED("cannot get medium attachment device"); + def->disks[diskCount].name = vboxGenerateMediumName(storageBus, + deviceInst, + devicePort, + deviceSlot, + maxPortPerInst, + maxSlotPerPort); + } + VBOX_UTF8_FREE(diskSnapIdStr); + } + } + VBOX_RELEASE(storageController); + VBOX_RELEASE(disk); + diskCount++; + } + vboxArrayRelease(&mediumAttachments); + /* cleanup on error */ + if (error) { + for (i = 0; i < def->dom->ndisks; i++) { + VIR_FREE(def->dom->disks[i]); + } + VIR_FREE(def->dom->disks); + def->dom->ndisks = 0; + return -1; + } + return 0; + +cleanup: + VBOX_RELEASE(snap); + return ret; +} + +static +int vboxSnapshotGetReadOnlyDisks(virDomainSnapshotPtr snapshot, + virDomainSnapshotDefPtr def) +{ + virDomainPtr dom = snapshot->domain; + VBOX_OBJECT_CHECK(dom->conn, int, -1); + vboxIID domiid = VBOX_IID_INITIALIZER; + ISnapshot *snap = NULL; + IMachine *machine = NULL; + IMachine *snapMachine = NULL; + IStorageController *storageController = NULL; + IMedium *disk = NULL; + nsresult rc; + vboxIIDFromUUID(&domiid, dom->uuid); + bool error = false; + vboxArray mediumAttachments = VBOX_ARRAY_INITIALIZER; + size_t i = 0; + PRUint32 maxPortPerInst[StorageBus_Floppy + 1] = {}; + PRUint32 maxSlotPerPort[StorageBus_Floppy + 1] = {}; + int diskCount = 0; + rc = VBOX_OBJECT_GET_MACHINE(domiid.value, &machine); + if (NS_FAILED(rc)) { + virReportError(VIR_ERR_NO_DOMAIN, "%s", + _("no domain with matching UUID")); + return -1; + } + + if (!(snap = vboxDomainSnapshotGet(data, dom, machine, snapshot->name))) + return -1; + rc = snap->vtbl->GetMachine(snap, &snapMachine); + reportInternalErrorIfNS_FAILED("cannot get machine"); + /* + * Get READ ONLY disks + * In the snapshot metadata, these are the disks written inside the <domain> node + */ + rc = vboxArrayGet(&mediumAttachments, snapMachine, snapMachine->vtbl->GetMediumAttachments); + reportInternalErrorIfNS_FAILED("cannot get medium attachments"); + /* get the number of attachments */ + for (i = 0; i < mediumAttachments.count; i++) { + IMediumAttachment *imediumattach = mediumAttachments.items[i]; + if (imediumattach) { + IMedium *medium = NULL; + + rc = imediumattach->vtbl->GetMedium(imediumattach, &medium); + reportInternalErrorIfNS_FAILED("cannot get medium"); + if (medium) { + def->dom->ndisks++; + VBOX_RELEASE(medium); + } + } + } + + /* Allocate mem, if fails return error */ + if (VIR_ALLOC_N(def->dom->disks, def->dom->ndisks) >= 0) { + for (i = 0; i < def->dom->ndisks; i++) { + if (VIR_ALLOC(def->dom->disks[i]) < 0) { + virReportOOMError(); + error = true; + break; + } + } + } else { + virReportOOMError(); + error = true; + } + + if (!error) + error = !vboxGetMaxPortSlotValues(data->vboxObj, maxPortPerInst, maxSlotPerPort); + + /* get the attachment details here */ + for (i = 0; i < mediumAttachments.count && diskCount < def->dom->ndisks && !error; i++) { + PRUnichar *storageControllerName = NULL; + PRUint32 deviceType = DeviceType_Null; + PRUint32 storageBus = StorageBus_Null; + PRBool readOnly = PR_FALSE; + PRUnichar *mediumLocUtf16 = NULL; + char *mediumLocUtf8 = NULL; + PRUint32 deviceInst = 0; + PRInt32 devicePort = 0; + PRInt32 deviceSlot = 0; + IMediumAttachment *imediumattach = mediumAttachments.items[i]; + if (!imediumattach) + continue; + rc = imediumattach->vtbl->GetMedium(imediumattach, &disk); + reportInternalErrorIfNS_FAILED("cannot get medium"); + if (!disk) + continue; + rc = imediumattach->vtbl->GetController(imediumattach, &storageControllerName); + reportInternalErrorIfNS_FAILED("cannot get storage controller name"); + if (!storageControllerName) { + continue; + } + rc = machine->vtbl->GetStorageControllerByName(machine, + storageControllerName, + &storageController); + reportInternalErrorIfNS_FAILED("cannot get storage controller"); + VBOX_UTF16_FREE(storageControllerName); + if (!storageController) { + continue; + } + rc = disk->vtbl->GetLocation(disk, &mediumLocUtf16); + reportInternalErrorIfNS_FAILED("cannot get disk location"); + VBOX_UTF16_TO_UTF8(mediumLocUtf16, &mediumLocUtf8); + VBOX_UTF16_FREE(mediumLocUtf16); + ignore_value(VIR_STRDUP(def->dom->disks[diskCount]->src, mediumLocUtf8)); + + if (!(def->dom->disks[diskCount]->src)) { + virReportOOMError(); + ret = -1; + goto cleanup; + } + VBOX_UTF8_FREE(mediumLocUtf8); + + rc = storageController->vtbl->GetBus(storageController, &storageBus); + reportInternalErrorIfNS_FAILED("cannot get storage controller bus"); + if (storageBus == StorageBus_IDE) { + def->dom->disks[diskCount]->bus = VIR_DOMAIN_DISK_BUS_IDE; + } else if (storageBus == StorageBus_SATA) { + def->dom->disks[diskCount]->bus = VIR_DOMAIN_DISK_BUS_SATA; + } else if (storageBus == StorageBus_SCSI) { + def->dom->disks[diskCount]->bus = VIR_DOMAIN_DISK_BUS_SCSI; + } else if (storageBus == StorageBus_Floppy) { + def->dom->disks[diskCount]->bus = VIR_DOMAIN_DISK_BUS_FDC; + } + + rc = imediumattach->vtbl->GetType(imediumattach, &deviceType); + reportInternalErrorIfNS_FAILED("cannot get medium attachment type"); + if (deviceType == DeviceType_HardDisk) + def->dom->disks[diskCount]->device = VIR_DOMAIN_DISK_DEVICE_DISK; + else if (deviceType == DeviceType_Floppy) + def->dom->disks[diskCount]->device = VIR_DOMAIN_DISK_DEVICE_FLOPPY; + else if (deviceType == DeviceType_DVD) + def->dom->disks[diskCount]->device = VIR_DOMAIN_DISK_DEVICE_CDROM; + + rc = imediumattach->vtbl->GetPort(imediumattach, &devicePort); + reportInternalErrorIfNS_FAILED("cannot get medium attachment port"); + rc = imediumattach->vtbl->GetDevice(imediumattach, &deviceSlot); + reportInternalErrorIfNS_FAILED("cannot get device"); + rc = disk->vtbl->GetReadOnly(disk, &readOnly); + reportInternalErrorIfNS_FAILED("cannot get read only attribute"); + if (readOnly == PR_TRUE) + def->dom->disks[diskCount]->readonly = true; + def->dom->disks[diskCount]->type = VIR_DOMAIN_DISK_TYPE_FILE; + def->dom->disks[diskCount]->dst = vboxGenerateMediumName(storageBus, + deviceInst, + devicePort, + deviceSlot, + maxPortPerInst, + maxSlotPerPort); + if (!def->dom->disks[diskCount]->dst) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Could not generate medium name for the disk " + "at: controller instance:%u, port:%d, slot:%d"), + deviceInst, devicePort, deviceSlot); + ret = -1; + goto cleanup; + } + diskCount ++; + } + /* cleanup on error */ + if (error) { + for (i = 0; i < def->dom->ndisks; i++) { + VIR_FREE(def->dom->disks[i]); + } + VIR_FREE(def->dom->disks); + def->dom->ndisks = 0; + ret = -1; + goto cleanup; + } + ret = 0; +cleanup: + VBOX_RELEASE(disk); + VBOX_RELEASE(storageController); + vboxArrayRelease(&mediumAttachments); + VBOX_RELEASE(snap); + return ret; +} +#endif + static char * vboxDomainSnapshotGetXMLDesc(virDomainSnapshotPtr snapshot, unsigned int flags) @@ -6065,6 +6417,10 @@ vboxDomainSnapshotGetXMLDesc(virDomainSnapshotPtr snapshot, PRInt64 timestamp; PRBool online = PR_FALSE; char uuidstr[VIR_UUID_STRING_BUFLEN]; +#if VBOX_API_VERSION >=4002 + PRUint32 memorySize = 0; + PRUint32 CPUCount = 0; +#endif virCheckFlags(0, NULL); @@ -6079,11 +6435,40 @@ vboxDomainSnapshotGetXMLDesc(virDomainSnapshotPtr snapshot, if (!(snap = vboxDomainSnapshotGet(data, dom, machine, snapshot->name))) goto cleanup; - if (VIR_ALLOC(def) < 0) + if (VIR_ALLOC(def) < 0 + || VIR_ALLOC(def->dom) < 0) goto cleanup; if (VIR_STRDUP(def->name, snapshot->name) < 0) goto cleanup; +#if VBOX_API_VERSION >=4002 + /* Register def->dom properties for them to be saved inside the snapshot XMl + * Otherwise, there is a problem while parsing the xml + */ + def->dom->virtType = VIR_DOMAIN_VIRT_VBOX; + def->dom->id = dom->id; + memcpy(def->dom->uuid, dom->uuid, VIR_UUID_BUFLEN); + ignore_value(VIR_STRDUP(def->dom->name, dom->name)); + machine->vtbl->GetMemorySize(machine, &memorySize); + def->dom->mem.cur_balloon = memorySize * 1024; + /* Currently setting memory and maxMemory as same, cause + * the notation here seems to be inconsistent while + * reading and while dumping xml + */ + def->dom->mem.max_balloon = memorySize * 1024; + ignore_value(VIR_STRDUP(def->dom->os.type, "hvm")); + def->dom->os.arch = virArchFromHost(); + machine->vtbl->GetCPUCount(machine, &CPUCount); + def->dom->maxvcpus = def->dom->vcpus = CPUCount; + if (vboxSnapshotGetReadWriteDisks(def, snapshot) < 0) { + VIR_DEBUG("Could not get read write disks for snapshot"); + } + + if (vboxSnapshotGetReadOnlyDisks(snapshot, def) <0) { + VIR_DEBUG("Could not get Readonly disks for snapshot"); + } +#endif /* VBOX_API_VERSION >= 4002 */ + rc = snap->vtbl->GetDescription(snap, &str16); if (NS_FAILED(rc)) { virReportError(VIR_ERR_INTERNAL_ERROR, @@ -6148,8 +6533,8 @@ vboxDomainSnapshotGetXMLDesc(virDomainSnapshotPtr snapshot, def->state = VIR_DOMAIN_SHUTOFF; virUUIDFormat(dom->uuid, uuidstr); + memcpy(def->dom->uuid, dom->uuid, VIR_UUID_BUFLEN); ret = virDomainSnapshotDefFormat(uuidstr, def, flags, 0); - cleanup: virDomainSnapshotDefFree(def); VBOX_RELEASE(parent); @@ -6854,6 +7239,8 @@ cleanup: return ret; } + + static int vboxDomainSnapshotDelete(virDomainSnapshotPtr snapshot, unsigned int flags) @@ -6889,8 +7276,9 @@ vboxDomainSnapshotDelete(virDomainSnapshotPtr snapshot, goto cleanup; } - /* VBOX snapshots do not require any libvirt metadata, making this - * flag trivial once we know we have a valid snapshot. */ + /* In case we just want to delete the metadata, we will edit the vbox file in order + *to remove the node concerning the snapshot + */ if (flags & VIR_DOMAIN_SNAPSHOT_DELETE_METADATA_ONLY) { ret = 0; goto cleanup; @@ -8716,8 +9104,9 @@ cleanup: return ret; } -static int vboxStorageVolDelete(virStorageVolPtr vol, - unsigned int flags) +static int vboxStorageDeleteOrClose(virStorageVolPtr vol, + unsigned int flags, + unsigned int flagDeleteOrClose) { VBOX_OBJECT_CHECK(vol->conn, int, -1); vboxIID hddIID = VBOX_IID_INITIALIZER; @@ -8872,8 +9261,18 @@ static int vboxStorageVolDelete(virStorageVolPtr vol, if (machineIdsSize == 0 || machineIdsSize == deregister) { IProgress *progress = NULL; - +#if VBOX_API_VERSION >= 4002 + if (flagDeleteOrClose & VBOX_STORAGE_CLOSE_FLAG) { + rc = hardDisk->vtbl->Close(hardDisk); + if (NS_SUCCEEDED(rc)) { + DEBUGIID("HardDisk closed, UUID", hddIID.value); + ret = 0; + } + } +#endif + if (flagDeleteOrClose & VBOX_STORAGE_DELETE_FLAG){ rc = hardDisk->vtbl->DeleteStorage(hardDisk, &progress); + } if (NS_SUCCEEDED(rc) && progress) { progress->vtbl->WaitForCompletion(progress, -1); @@ -8892,6 +9291,12 @@ static int vboxStorageVolDelete(virStorageVolPtr vol, return ret; } +static int vboxStorageVolDelete(virStorageVolPtr vol, + unsigned int flags) +{ + return vboxStorageDeleteOrClose(vol, flags, VBOX_STORAGE_DELETE_FLAG); +} + static int vboxStorageVolGetInfo(virStorageVolPtr vol, virStorageVolInfoPtr info) { VBOX_OBJECT_CHECK(vol->conn, int, -1); IHardDisk *hardDisk = NULL; -- 1.7.10.4
On 11/25/2013 07:23 PM, Manuel VIVES wrote:
It will be needed for the futur patches because we will redefine snapshots
s/futur/future/ :-)
--- src/conf/domain_conf.c | 2 +- src/vbox/vbox_tmpl.c | 427 ++++++++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 417 insertions(+), 12 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 7b0e3ea..ae20eb5 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -17054,7 +17054,7 @@ virDomainDefFormatInternal(virDomainDefPtr def, if (virDomainChrDefFormat(buf, &console, flags) < 0) goto error; } - if (STREQ(def->os.type, "hvm") && + if (STREQ_NULLABLE(def->os.type, "hvm") && def->nconsoles == 0 && def->nserials > 0) { virDomainChrDef console;
This hunk should be a separate patch. BTW, is it valid for vbox domains to not have a valid os.type? virDomainDefPostParseInternal() requires it to be set, and there are several other places in domain_conf.c that compare it without allowing for NULL, so if NULL is valid, those other places should at least get scrutiny, or you should make sure that it's set when you construct the domain object in the vbox driver.
diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c index 983a595..e05ed97 100644 --- a/src/vbox/vbox_tmpl.c +++ b/src/vbox/vbox_tmpl.c @@ -38,6 +38,7 @@ #include <sys/types.h> #include <sys/stat.h> #include <fcntl.h> +#include <libxml/xmlwriter.h>
#include "internal.h" #include "datatypes.h" @@ -58,6 +59,8 @@ #include "fdstream.h" #include "viruri.h" #include "virstring.h" +#include "virtime.h" +#include "virutil.h"
/* This one changes from version to version. */ #if VBOX_API_VERSION == 2002 @@ -273,10 +276,16 @@ static vboxGlobalData *g_pVBoxGlobalData = NULL;
#endif /* VBOX_API_VERSION >= 4000 */
+#define reportInternalErrorIfNS_FAILED(message) \ + if (NS_FAILED(rc)) { \ + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _(message)); \ + goto cleanup; \ + } + +
I would prefer these if() clauses with goto to be explicitly in the code, rather than hidden in a macro. That way it's easier to see all potential code paths when scanning through the code. If you have a goto hidden in a macro, then it's not immediately visible and that can only lead to problems.
static virDomainPtr vboxDomainDefineXML(virConnectPtr conn, const char *xml); static int vboxDomainCreate(virDomainPtr dom); static int vboxDomainUndefineFlags(virDomainPtr dom, unsigned int flags); - static void vboxDriverLock(vboxGlobalData *data) {
You should leave in that blank line, as it separates static function declarations from a function definition.
virMutexLock(&data->lock); } @@ -285,6 +294,12 @@ static void vboxDriverUnlock(vboxGlobalData *data) { virMutexUnlock(&data->lock); }
+typedef enum { + VBOX_STORAGE_DELETE_FLAG = 0, +#if VBOX_API_VERSION >= 4002 + VBOX_STORAGE_CLOSE_FLAG = 1, +#endif +} vboxStorageDeleteOrCloseFlags; #if VBOX_API_VERSION == 2002
static void nsIDtoChar(unsigned char *uuid, const nsID *iid) { @@ -5957,7 +5972,8 @@ vboxDomainSnapshotCreateXML(virDomainPtr dom, virCheckFlags(VIR_DOMAIN_SNAPSHOT_CREATE_NO_METADATA, NULL);
if (!(def = virDomainSnapshotDefParseString(xmlDesc, data->caps, - data->xmlopt, 0, 0))) + data->xmlopt, -1, VIR_DOMAIN_SNAPSHOT_PARSE_DISKS | + VIR_DOMAIN_SNAPSHOT_PARSE_REDEFINE)))
Move the VIR_DOMAIN_SNAPSHOT... down one line so that it can be moved left and hopefully fit within 80 columns.
goto cleanup;
if (def->ndisks) { @@ -5965,7 +5981,6 @@ vboxDomainSnapshotCreateXML(virDomainPtr dom, _("disk snapshots not supported yet")); goto cleanup; } -
Any particular reason for removing this blank line (and the one below)?
vboxIIDFromUUID(&domiid, dom->uuid); rc = VBOX_OBJECT_GET_MACHINE(domiid.value, &machine); if (NS_FAILED(rc)) { @@ -5973,7 +5988,6 @@ vboxDomainSnapshotCreateXML(virDomainPtr dom, _("no domain with matching UUID")); goto cleanup; } - rc = machine->vtbl->GetState(machine, &state); if (NS_FAILED(rc)) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", @@ -6048,6 +6062,344 @@ cleanup: return ret; }
+#if VBOX_API_VERSION >=4002 +static +int vboxSnapshotGetReadWriteDisks(virDomainSnapshotDefPtr def, + virDomainSnapshotPtr snapshot) +{ + virDomainPtr dom = snapshot->domain; + VBOX_OBJECT_CHECK(dom->conn, int, -1); + vboxIID domiid = VBOX_IID_INITIALIZER; + IMachine *machine = NULL; + ISnapshot *snap = NULL; + IMachine *snapMachine = NULL; + bool error = false;
The combination of having "error" and "ret" is confusing, and the possibility at the bottom of the function of either going to cleanup to return "ret" or dropping through to check for error == true and then return 0 or -1 is also confusing. You can probably make it much simpler by removing error, initializing ret = -1, then when you get to cleanup at the end, just check for ret < 0 and do the error cleanup in that case. All of the places you set "error = true" would then just be "goto cleanup" (I'm not sure why your current code continues to process once it has encountered an error, but I'm making the assumption that is superfluous and not intentional). For the case of success, you would just drop through to the cleanup label, and just before that would be a "ret = 0;". This is the way a vast majority of functions in libvirt are written, and it works very well for us in most cases.
+ vboxArray mediumAttachments = VBOX_ARRAY_INITIALIZER; + PRUint32 maxPortPerInst[StorageBus_Floppy + 1] = {}; + PRUint32 maxSlotPerPort[StorageBus_Floppy + 1] = {}; + int diskCount = 0; + nsresult rc; + vboxIID snapIid = VBOX_IID_INITIALIZER; + char *snapshotUuidStr = NULL; + size_t i = 0; + vboxIIDFromUUID(&domiid, dom->uuid); + rc = VBOX_OBJECT_GET_MACHINE(domiid.value, &machine); + reportInternalErrorIfNS_FAILED("no domain with matching UUID"); + if (!(snap = vboxDomainSnapshotGet(data, dom, machine, snapshot->name))) { + ret = -1; + goto cleanup; + } + rc = snap->vtbl->GetId(snap, &snapIid.value); + reportInternalErrorIfNS_FAILED("Could not get snapshot id"); + + VBOX_UTF16_TO_UTF8(snapIid.value, &snapshotUuidStr); + rc = snap->vtbl->GetMachine(snap, &snapMachine); + reportInternalErrorIfNS_FAILED("could not get machine"); + def->ndisks = 0; + rc = vboxArrayGet(&mediumAttachments, snapMachine, snapMachine->vtbl->GetMediumAttachments); + reportInternalErrorIfNS_FAILED("no medium attachments"); + /* get the number of attachments */ + for (i = 0; i < mediumAttachments.count; i++) { + IMediumAttachment *imediumattach = mediumAttachments.items[i]; + if (imediumattach) { + IMedium *medium = NULL; + + rc = imediumattach->vtbl->GetMedium(imediumattach, &medium); + reportInternalErrorIfNS_FAILED("cannot get medium"); + if (medium) { + def->ndisks++; + VBOX_RELEASE(medium); + } + } + } + /* Allocate mem, if fails return error */ + if (VIR_ALLOC_N(def->disks, def->ndisks) < 0) { + virReportOOMError();
You no longer need to report an error if VIR_ALLOC_N() fails - it's done for you; just return failure to your caller.
+ error = true; + } + if (!error) + error = !vboxGetMaxPortSlotValues(data->vboxObj, maxPortPerInst, maxSlotPerPort); + + /* get the attachment details here */ + for (i = 0; i < mediumAttachments.count && diskCount < def->ndisks && !error; i++) { + IStorageController *storageController = NULL; + PRUnichar *storageControllerName = NULL; + PRUint32 deviceType = DeviceType_Null; + PRUint32 storageBus = StorageBus_Null; + IMedium *disk = NULL; + PRUnichar *childLocUtf16 = NULL; + char *childLocUtf8 = NULL; + PRUint32 deviceInst = 0; + PRInt32 devicePort = 0; + PRInt32 deviceSlot = 0; + vboxArray children = VBOX_ARRAY_INITIALIZER; + vboxArray snapshotIids = VBOX_ARRAY_INITIALIZER; + IMediumAttachment *imediumattach = mediumAttachments.items[i]; + size_t j = 0; + size_t k = 0; + if (!imediumattach) + continue; + rc = imediumattach->vtbl->GetMedium(imediumattach, &disk); + reportInternalErrorIfNS_FAILED("cannot get medium"); + if (!disk) + continue; + rc = imediumattach->vtbl->GetController(imediumattach, &storageControllerName); + reportInternalErrorIfNS_FAILED("cannot get medium"); + if (!storageControllerName) { + VBOX_RELEASE(disk); + continue; + } + rc= vboxArrayGet(&children, disk, disk->vtbl->GetChildren); + reportInternalErrorIfNS_FAILED("cannot get children disk"); + rc = vboxArrayGetWithPtrArg(&snapshotIids, disk, disk->vtbl->GetSnapshotIds, domiid.value); + reportInternalErrorIfNS_FAILED("cannot get snapshot ids"); + for (j = 0; j < children.count; ++j) { + IMedium *child = children.items[j]; + for (k = 0; k < snapshotIids.count; ++k) { + PRUnichar *diskSnapId = snapshotIids.items[k]; + char *diskSnapIdStr = NULL; + VBOX_UTF16_TO_UTF8(diskSnapId, &diskSnapIdStr); + if (STREQ(diskSnapIdStr, snapshotUuidStr)) { + rc = machine->vtbl->GetStorageControllerByName(machine, + storageControllerName, + &storageController); + VBOX_UTF16_FREE(storageControllerName); + if (!storageController) { + VBOX_RELEASE(child); + break; + } + rc = child->vtbl->GetLocation(child, &childLocUtf16); + reportInternalErrorIfNS_FAILED("cannot get disk location"); + VBOX_UTF16_TO_UTF8(childLocUtf16, &childLocUtf8); + VBOX_UTF16_FREE(childLocUtf16); + ignore_value(VIR_STRDUP(def->disks[diskCount].file, childLocUtf8)); + if (!(def->disks[diskCount].file)) { + VBOX_RELEASE(child); + VBOX_RELEASE(storageController); + virReportOOMError();
Same here - VIR_STRDUP() reports the error for you. (in the rare case that you *don't* want it automatically reported, you would call VIR_STRDUP_QUIET()). (I probably ignored other occurrences of this problem in your patches...)
+ error = true; + break; + } + VBOX_UTF8_FREE(childLocUtf8); + + rc = storageController->vtbl->GetBus(storageController, &storageBus); + reportInternalErrorIfNS_FAILED("cannot get storage controller bus"); + rc = imediumattach->vtbl->GetType(imediumattach, &deviceType); + reportInternalErrorIfNS_FAILED("cannot get medium attachment type"); + rc = imediumattach->vtbl->GetPort(imediumattach, &devicePort); + reportInternalErrorIfNS_FAILED("cannot get medium attachchment type"); + rc = imediumattach->vtbl->GetDevice(imediumattach, &deviceSlot); + reportInternalErrorIfNS_FAILED("cannot get medium attachment device"); + def->disks[diskCount].name = vboxGenerateMediumName(storageBus, + deviceInst, + devicePort, + deviceSlot, + maxPortPerInst, + maxSlotPerPort); + } + VBOX_UTF8_FREE(diskSnapIdStr); + } + } + VBOX_RELEASE(storageController); + VBOX_RELEASE(disk); + diskCount++; + } + vboxArrayRelease(&mediumAttachments); + /* cleanup on error */ + if (error) { + for (i = 0; i < def->dom->ndisks; i++) { + VIR_FREE(def->dom->disks[i]); + } + VIR_FREE(def->dom->disks); + def->dom->ndisks = 0; + return -1; + } + return 0; + +cleanup: + VBOX_RELEASE(snap); + return ret; +} + +static +int vboxSnapshotGetReadOnlyDisks(virDomainSnapshotPtr snapshot, + virDomainSnapshotDefPtr def) +{ + virDomainPtr dom = snapshot->domain; + VBOX_OBJECT_CHECK(dom->conn, int, -1); + vboxIID domiid = VBOX_IID_INITIALIZER; + ISnapshot *snap = NULL; + IMachine *machine = NULL; + IMachine *snapMachine = NULL; + IStorageController *storageController = NULL; + IMedium *disk = NULL; + nsresult rc; + vboxIIDFromUUID(&domiid, dom->uuid); + bool error = false; + vboxArray mediumAttachments = VBOX_ARRAY_INITIALIZER; + size_t i = 0; + PRUint32 maxPortPerInst[StorageBus_Floppy + 1] = {}; + PRUint32 maxSlotPerPort[StorageBus_Floppy + 1] = {}; + int diskCount = 0; + rc = VBOX_OBJECT_GET_MACHINE(domiid.value, &machine);
Put a blank line between the automatic data declarations and the first statement.
+ if (NS_FAILED(rc)) { + virReportError(VIR_ERR_NO_DOMAIN, "%s", + _("no domain with matching UUID")); + return -1; + }
Hmm. You defined a macro to do almost exactly this, but couldn't use it here because you couldn't goto cleanup. Are the vbox cleanup unctions/macros not safe to call when the data they're cleaning up is still NULL? (not that I think you should try to use that macro - I think it should be killed. I'm just curious why you didn't use it here)
+ + if (!(snap = vboxDomainSnapshotGet(data, dom, machine, snapshot->name))) + return -1; + rc = snap->vtbl->GetMachine(snap, &snapMachine); + reportInternalErrorIfNS_FAILED("cannot get machine"); + /* + * Get READ ONLY disks + * In the snapshot metadata, these are the disks written inside the <domain> node + */ + rc = vboxArrayGet(&mediumAttachments, snapMachine, snapMachine->vtbl->GetMediumAttachments); + reportInternalErrorIfNS_FAILED("cannot get medium attachments"); + /* get the number of attachments */ + for (i = 0; i < mediumAttachments.count; i++) { + IMediumAttachment *imediumattach = mediumAttachments.items[i]; + if (imediumattach) { + IMedium *medium = NULL; + + rc = imediumattach->vtbl->GetMedium(imediumattach, &medium); + reportInternalErrorIfNS_FAILED("cannot get medium"); + if (medium) { + def->dom->ndisks++; + VBOX_RELEASE(medium); + } + } + } + + /* Allocate mem, if fails return error */ + if (VIR_ALLOC_N(def->dom->disks, def->dom->ndisks) >= 0) { + for (i = 0; i < def->dom->ndisks; i++) { + if (VIR_ALLOC(def->dom->disks[i]) < 0) { + virReportOOMError(); + error = true;
Again, unneeded call to virReportOOMError(), and it would be better to have initialized ret = -1 at the top, then done "goto cleanup" here.
+ break; + } + } + } else { + virReportOOMError(); + error = true;
Likewise. I'll stop pointing these out now...
+ } + + if (!error) + error = !vboxGetMaxPortSlotValues(data->vboxObj, maxPortPerInst, maxSlotPerPort); + + /* get the attachment details here */ + for (i = 0; i < mediumAttachments.count && diskCount < def->dom->ndisks && !error; i++) {
You' going to a lot of trouble to avoid executing this code after error has be set to true, again indicating that it would be simpler to just goto cleanup immediately when you encounter an error (and have cleanup contain the proper code to cleanup from an error).
+ PRUnichar *storageControllerName = NULL; + PRUint32 deviceType = DeviceType_Null; + PRUint32 storageBus = StorageBus_Null; + PRBool readOnly = PR_FALSE; + PRUnichar *mediumLocUtf16 = NULL; + char *mediumLocUtf8 = NULL; + PRUint32 deviceInst = 0; + PRInt32 devicePort = 0; + PRInt32 deviceSlot = 0; + IMediumAttachment *imediumattach = mediumAttachments.items[i]; + if (!imediumattach) + continue; + rc = imediumattach->vtbl->GetMedium(imediumattach, &disk); + reportInternalErrorIfNS_FAILED("cannot get medium"); + if (!disk) + continue; + rc = imediumattach->vtbl->GetController(imediumattach, &storageControllerName); + reportInternalErrorIfNS_FAILED("cannot get storage controller name"); + if (!storageControllerName) { + continue; + } + rc = machine->vtbl->GetStorageControllerByName(machine, + storageControllerName, + &storageController); + reportInternalErrorIfNS_FAILED("cannot get storage controller"); + VBOX_UTF16_FREE(storageControllerName); + if (!storageController) { + continue; + } + rc = disk->vtbl->GetLocation(disk, &mediumLocUtf16); + reportInternalErrorIfNS_FAILED("cannot get disk location"); + VBOX_UTF16_TO_UTF8(mediumLocUtf16, &mediumLocUtf8); + VBOX_UTF16_FREE(mediumLocUtf16); + ignore_value(VIR_STRDUP(def->dom->disks[diskCount]->src, mediumLocUtf8)); + + if (!(def->dom->disks[diskCount]->src)) { + virReportOOMError(); + ret = -1; + goto cleanup; + } + VBOX_UTF8_FREE(mediumLocUtf8); + + rc = storageController->vtbl->GetBus(storageController, &storageBus); + reportInternalErrorIfNS_FAILED("cannot get storage controller bus"); + if (storageBus == StorageBus_IDE) { + def->dom->disks[diskCount]->bus = VIR_DOMAIN_DISK_BUS_IDE; + } else if (storageBus == StorageBus_SATA) { + def->dom->disks[diskCount]->bus = VIR_DOMAIN_DISK_BUS_SATA; + } else if (storageBus == StorageBus_SCSI) { + def->dom->disks[diskCount]->bus = VIR_DOMAIN_DISK_BUS_SCSI; + } else if (storageBus == StorageBus_Floppy) { + def->dom->disks[diskCount]->bus = VIR_DOMAIN_DISK_BUS_FDC; + } + + rc = imediumattach->vtbl->GetType(imediumattach, &deviceType); + reportInternalErrorIfNS_FAILED("cannot get medium attachment type"); + if (deviceType == DeviceType_HardDisk) + def->dom->disks[diskCount]->device = VIR_DOMAIN_DISK_DEVICE_DISK; + else if (deviceType == DeviceType_Floppy) + def->dom->disks[diskCount]->device = VIR_DOMAIN_DISK_DEVICE_FLOPPY; + else if (deviceType == DeviceType_DVD) + def->dom->disks[diskCount]->device = VIR_DOMAIN_DISK_DEVICE_CDROM; + + rc = imediumattach->vtbl->GetPort(imediumattach, &devicePort); + reportInternalErrorIfNS_FAILED("cannot get medium attachment port"); + rc = imediumattach->vtbl->GetDevice(imediumattach, &deviceSlot); + reportInternalErrorIfNS_FAILED("cannot get device"); + rc = disk->vtbl->GetReadOnly(disk, &readOnly); + reportInternalErrorIfNS_FAILED("cannot get read only attribute"); + if (readOnly == PR_TRUE) + def->dom->disks[diskCount]->readonly = true; + def->dom->disks[diskCount]->type = VIR_DOMAIN_DISK_TYPE_FILE; + def->dom->disks[diskCount]->dst = vboxGenerateMediumName(storageBus, + deviceInst, + devicePort, + deviceSlot, + maxPortPerInst, + maxSlotPerPort); + if (!def->dom->disks[diskCount]->dst) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Could not generate medium name for the disk " + "at: controller instance:%u, port:%d, slot:%d"), + deviceInst, devicePort, deviceSlot); + ret = -1; + goto cleanup; + } + diskCount ++; + } + /* cleanup on error */
... but not on *all* errors, since you sometimes set error = true and continue all the way down to here, and sometime you set set = -1 and goto cleanup (thus skipping this extra cleanup). ALL of the cleanup should be after the cleanup label, and you should always goto cleanup when you encounter an error. That way you won't have some error recovery paths that skip some of the cleanup.
+ if (error) { + for (i = 0; i < def->dom->ndisks; i++) { + VIR_FREE(def->dom->disks[i]); + } + VIR_FREE(def->dom->disks); + def->dom->ndisks = 0; + ret = -1; + goto cleanup; + } + ret = 0; +cleanup: + VBOX_RELEASE(disk); + VBOX_RELEASE(storageController); + vboxArrayRelease(&mediumAttachments); + VBOX_RELEASE(snap); + return ret; +} +#endif + static char * vboxDomainSnapshotGetXMLDesc(virDomainSnapshotPtr snapshot, unsigned int flags) @@ -6065,6 +6417,10 @@ vboxDomainSnapshotGetXMLDesc(virDomainSnapshotPtr snapshot, PRInt64 timestamp; PRBool online = PR_FALSE; char uuidstr[VIR_UUID_STRING_BUFLEN]; +#if VBOX_API_VERSION >=4002 + PRUint32 memorySize = 0; + PRUint32 CPUCount = 0; +#endif
virCheckFlags(0, NULL);
@@ -6079,11 +6435,40 @@ vboxDomainSnapshotGetXMLDesc(virDomainSnapshotPtr snapshot, if (!(snap = vboxDomainSnapshotGet(data, dom, machine, snapshot->name))) goto cleanup;
- if (VIR_ALLOC(def) < 0) + if (VIR_ALLOC(def) < 0 + || VIR_ALLOC(def->dom) < 0) goto cleanup; if (VIR_STRDUP(def->name, snapshot->name) < 0) goto cleanup;
+#if VBOX_API_VERSION >=4002 + /* Register def->dom properties for them to be saved inside the snapshot XMl + * Otherwise, there is a problem while parsing the xml + */ + def->dom->virtType = VIR_DOMAIN_VIRT_VBOX; + def->dom->id = dom->id; + memcpy(def->dom->uuid, dom->uuid, VIR_UUID_BUFLEN); + ignore_value(VIR_STRDUP(def->dom->name, dom->name)); + machine->vtbl->GetMemorySize(machine, &memorySize); + def->dom->mem.cur_balloon = memorySize * 1024; + /* Currently setting memory and maxMemory as same, cause + * the notation here seems to be inconsistent while + * reading and while dumping xml + */ + def->dom->mem.max_balloon = memorySize * 1024; + ignore_value(VIR_STRDUP(def->dom->os.type, "hvm")); + def->dom->os.arch = virArchFromHost(); + machine->vtbl->GetCPUCount(machine, &CPUCount); + def->dom->maxvcpus = def->dom->vcpus = CPUCount; + if (vboxSnapshotGetReadWriteDisks(def, snapshot) < 0) { + VIR_DEBUG("Could not get read write disks for snapshot"); + } + + if (vboxSnapshotGetReadOnlyDisks(snapshot, def) <0) { + VIR_DEBUG("Could not get Readonly disks for snapshot"); + } +#endif /* VBOX_API_VERSION >= 4002 */ + rc = snap->vtbl->GetDescription(snap, &str16); if (NS_FAILED(rc)) { virReportError(VIR_ERR_INTERNAL_ERROR, @@ -6148,8 +6533,8 @@ vboxDomainSnapshotGetXMLDesc(virDomainSnapshotPtr snapshot, def->state = VIR_DOMAIN_SHUTOFF;
virUUIDFormat(dom->uuid, uuidstr); + memcpy(def->dom->uuid, dom->uuid, VIR_UUID_BUFLEN); ret = virDomainSnapshotDefFormat(uuidstr, def, flags, 0); - cleanup: virDomainSnapshotDefFree(def); VBOX_RELEASE(parent); @@ -6854,6 +7239,8 @@ cleanup: return ret; }
+ + static int vboxDomainSnapshotDelete(virDomainSnapshotPtr snapshot, unsigned int flags) @@ -6889,8 +7276,9 @@ vboxDomainSnapshotDelete(virDomainSnapshotPtr snapshot, goto cleanup; }
- /* VBOX snapshots do not require any libvirt metadata, making this - * flag trivial once we know we have a valid snapshot. */ + /* In case we just want to delete the metadata, we will edit the vbox file in order + *to remove the node concerning the snapshot + */ if (flags & VIR_DOMAIN_SNAPSHOT_DELETE_METADATA_ONLY) { ret = 0; goto cleanup; @@ -8716,8 +9104,9 @@ cleanup: return ret; }
-static int vboxStorageVolDelete(virStorageVolPtr vol, - unsigned int flags) +static int vboxStorageDeleteOrClose(virStorageVolPtr vol, + unsigned int flags, + unsigned int flagDeleteOrClose) { VBOX_OBJECT_CHECK(vol->conn, int, -1); vboxIID hddIID = VBOX_IID_INITIALIZER; @@ -8872,8 +9261,18 @@ static int vboxStorageVolDelete(virStorageVolPtr vol,
if (machineIdsSize == 0 || machineIdsSize == deregister) { IProgress *progress = NULL; - +#if VBOX_API_VERSION >= 4002 + if (flagDeleteOrClose & VBOX_STORAGE_CLOSE_FLAG) { + rc = hardDisk->vtbl->Close(hardDisk); + if (NS_SUCCEEDED(rc)) { + DEBUGIID("HardDisk closed, UUID", hddIID.value); + ret = 0; + } + } +#endif + if (flagDeleteOrClose & VBOX_STORAGE_DELETE_FLAG){ rc = hardDisk->vtbl->DeleteStorage(hardDisk, &progress); + }
if (NS_SUCCEEDED(rc) && progress) { progress->vtbl->WaitForCompletion(progress, -1); @@ -8892,6 +9291,12 @@ static int vboxStorageVolDelete(virStorageVolPtr vol, return ret; }
+static int vboxStorageVolDelete(virStorageVolPtr vol, + unsigned int flags) +{ + return vboxStorageDeleteOrClose(vol, flags, VBOX_STORAGE_DELETE_FLAG); +} + static int vboxStorageVolGetInfo(virStorageVolPtr vol, virStorageVolInfoPtr info) { VBOX_OBJECT_CHECK(vol->conn, int, -1); IHardDisk *hardDisk = NULL;
On 11/25/2013 07:23 PM, Manuel VIVES wrote:
It will be needed for the futur patches because we will redefine snapshots
s/futur/future/ :-)
---
src/conf/domain_conf.c | 2 +- src/vbox/vbox_tmpl.c | 427 ++++++++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 417 insertions(+), 12 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 7b0e3ea..ae20eb5 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -17054,7 +17054,7 @@ virDomainDefFormatInternal(virDomainDefPtr def,
if (virDomainChrDefFormat(buf, &console, flags) < 0)
goto error;
}
- if (STREQ(def->os.type, "hvm") && + if (STREQ_NULLABLE(def->os.type, "hvm") &&
def->nconsoles == 0 && def->nserials > 0) { virDomainChrDef console;
This hunk should be a separate patch. BTW, is it valid for vbox domains to not have a valid os.type? virDomainDefPostParseInternal() requires it to be set, and there are several other places in domain_conf.c that compare it without allowing for NULL, so if NULL is valid, those other places should at least get scrutiny, or you should make sure that it's set when you construct the domain object in the vbox driver.
This portion was safely removed from the patch.
diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c index 983a595..e05ed97 100644 --- a/src/vbox/vbox_tmpl.c +++ b/src/vbox/vbox_tmpl.c @@ -38,6 +38,7 @@
#include <sys/types.h> #include <sys/stat.h> #include <fcntl.h>
+#include <libxml/xmlwriter.h>
#include "internal.h" #include "datatypes.h"
@@ -58,6 +59,8 @@
#include "fdstream.h" #include "viruri.h" #include "virstring.h"
+#include "virtime.h" +#include "virutil.h"
/* This one changes from version to version. */ #if VBOX_API_VERSION == 2002
@@ -273,10 +276,16 @@ static vboxGlobalData *g_pVBoxGlobalData = NULL;
#endif /* VBOX_API_VERSION >= 4000 */
+#define reportInternalErrorIfNS_FAILED(message) \ + if (NS_FAILED(rc)) { \ + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _(message)); \ + goto cleanup; \ + } + +
I would prefer these if() clauses with goto to be explicitly in the code, rather than hidden in a macro. That way it's easier to see all potential code paths when scanning through the code. If you have a goto hidden in a macro, then it's not immediately visible and that can only lead to problems.
I removed this macro in order to have 'goto' instructions clearly visible but it increases the size of the patch.
static virDomainPtr vboxDomainDefineXML(virConnectPtr conn, const char *xml); static int vboxDomainCreate(virDomainPtr dom); static int vboxDomainUndefineFlags(virDomainPtr dom, unsigned int flags);
-
static void vboxDriverLock(vboxGlobalData *data) {
You should leave in that blank line, as it separates static function declarations from a function definition.
virMutexLock(&data->lock);
}
@@ -285,6 +294,12 @@ static void vboxDriverUnlock(vboxGlobalData *data) {
virMutexUnlock(&data->lock);
}
+typedef enum { + VBOX_STORAGE_DELETE_FLAG = 0, +#if VBOX_API_VERSION >= 4002 + VBOX_STORAGE_CLOSE_FLAG = 1, +#endif +} vboxStorageDeleteOrCloseFlags;
#if VBOX_API_VERSION == 2002
static void nsIDtoChar(unsigned char *uuid, const nsID *iid) {
@@ -5957,7 +5972,8 @@ vboxDomainSnapshotCreateXML(virDomainPtr dom,
virCheckFlags(VIR_DOMAIN_SNAPSHOT_CREATE_NO_METADATA, NULL);
if (!(def = virDomainSnapshotDefParseString(xmlDesc, data->caps,
- data->xmlopt, 0, 0))) + data->xmlopt, -1, VIR_DOMAIN_SNAPSHOT_PARSE_DISKS | + VIR_DOMAIN_SNAPSHOT_PARSE_REDEFINE)))
Move the VIR_DOMAIN_SNAPSHOT... down one line so that it can be moved left and hopefully fit within 80 columns.
goto cleanup;
if (def->ndisks) {
@@ -5965,7 +5981,6 @@ vboxDomainSnapshotCreateXML(virDomainPtr dom,
_("disk snapshots not supported yet"));
goto cleanup;
}
-
Any particular reason for removing this blank line (and the one below)?
vboxIIDFromUUID(&domiid, dom->uuid); rc = VBOX_OBJECT_GET_MACHINE(domiid.value, &machine); if (NS_FAILED(rc)) {
@@ -5973,7 +5988,6 @@ vboxDomainSnapshotCreateXML(virDomainPtr dom,
_("no domain with matching UUID"));
goto cleanup;
}
-
rc = machine->vtbl->GetState(machine, &state); if (NS_FAILED(rc)) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
@@ -6048,6 +6062,344 @@ cleanup: return ret;
}
+#if VBOX_API_VERSION >=4002 +static +int vboxSnapshotGetReadWriteDisks(virDomainSnapshotDefPtr def, + virDomainSnapshotPtr snapshot) +{ + virDomainPtr dom = snapshot->domain; + VBOX_OBJECT_CHECK(dom->conn, int, -1); + vboxIID domiid = VBOX_IID_INITIALIZER; + IMachine *machine = NULL; + ISnapshot *snap = NULL; + IMachine *snapMachine = NULL; + bool error = false;
The combination of having "error" and "ret" is confusing, and the possibility at the bottom of the function of either going to cleanup to return "ret" or dropping through to check for error == true and then return 0 or -1 is also confusing. You can probably make it much simpler by removing error, initializing ret = -1, then when you get to cleanup at the end, just check for ret < 0 and do the error cleanup in that case.
All of the places you set "error = true" would then just be "goto cleanup" (I'm not sure why your current code continues to process once it has encountered an error, but I'm making the assumption that is superfluous and not intentional).
For the case of success, you would just drop through to the cleanup label, and just before that would be a "ret = 0;". This is the way a vast majority of functions in libvirt are written, and it works very well for us in most cases.
+ vboxArray mediumAttachments = VBOX_ARRAY_INITIALIZER; + PRUint32 maxPortPerInst[StorageBus_Floppy + 1] = {}; + PRUint32 maxSlotPerPort[StorageBus_Floppy + 1] = {}; + int diskCount = 0; + nsresult rc; + vboxIID snapIid = VBOX_IID_INITIALIZER; + char *snapshotUuidStr = NULL; + size_t i = 0; + vboxIIDFromUUID(&domiid, dom->uuid); + rc = VBOX_OBJECT_GET_MACHINE(domiid.value, &machine); + reportInternalErrorIfNS_FAILED("no domain with matching UUID"); + if (!(snap = vboxDomainSnapshotGet(data, dom, machine, snapshot->name))) { + ret = -1; + goto cleanup; + } + rc = snap->vtbl->GetId(snap, &snapIid.value); + reportInternalErrorIfNS_FAILED("Could not get snapshot id"); + + VBOX_UTF16_TO_UTF8(snapIid.value, &snapshotUuidStr); + rc = snap->vtbl->GetMachine(snap, &snapMachine); + reportInternalErrorIfNS_FAILED("could not get machine"); + def->ndisks = 0; + rc = vboxArrayGet(&mediumAttachments, snapMachine, snapMachine->vtbl->GetMediumAttachments); + reportInternalErrorIfNS_FAILED("no medium attachments"); + /* get the number of attachments */ + for (i = 0; i < mediumAttachments.count; i++) { + IMediumAttachment *imediumattach = mediumAttachments.items[i]; + if (imediumattach) { + IMedium *medium = NULL; + + rc = imediumattach->vtbl->GetMedium(imediumattach, &medium); + reportInternalErrorIfNS_FAILED("cannot get medium"); + if (medium) { + def->ndisks++; + VBOX_RELEASE(medium); + } + } + } + /* Allocate mem, if fails return error */ + if (VIR_ALLOC_N(def->disks, def->ndisks) < 0) { + virReportOOMError();
You no longer need to report an error if VIR_ALLOC_N() fails - it's done for you; just return failure to your caller.
+ error = true; + } + if (!error) + error = !vboxGetMaxPortSlotValues(data->vboxObj, maxPortPerInst, maxSlotPerPort); + + /* get the attachment details here */ + for (i = 0; i < mediumAttachments.count && diskCount < def->ndisks && !error; i++) { + IStorageController *storageController = NULL; + PRUnichar *storageControllerName = NULL; + PRUint32 deviceType = DeviceType_Null; + PRUint32 storageBus = StorageBus_Null; + IMedium *disk = NULL; + PRUnichar *childLocUtf16 = NULL; + char *childLocUtf8 = NULL; + PRUint32 deviceInst = 0; + PRInt32 devicePort = 0; + PRInt32 deviceSlot = 0; + vboxArray children = VBOX_ARRAY_INITIALIZER; + vboxArray snapshotIids = VBOX_ARRAY_INITIALIZER; + IMediumAttachment *imediumattach = mediumAttachments.items[i]; + size_t j = 0; + size_t k = 0; + if (!imediumattach) + continue; + rc = imediumattach->vtbl->GetMedium(imediumattach, &disk); + reportInternalErrorIfNS_FAILED("cannot get medium"); + if (!disk) + continue; + rc = imediumattach->vtbl->GetController(imediumattach, &storageControllerName); + reportInternalErrorIfNS_FAILED("cannot get medium"); + if (!storageControllerName) { + VBOX_RELEASE(disk); + continue; + } + rc= vboxArrayGet(&children, disk, disk->vtbl->GetChildren); + reportInternalErrorIfNS_FAILED("cannot get children disk"); + rc = vboxArrayGetWithPtrArg(&snapshotIids, disk, disk->vtbl->GetSnapshotIds, domiid.value); + reportInternalErrorIfNS_FAILED("cannot get snapshot ids"); + for (j = 0; j < children.count; ++j) { + IMedium *child = children.items[j]; + for (k = 0; k < snapshotIids.count; ++k) { + PRUnichar *diskSnapId = snapshotIids.items[k]; + char *diskSnapIdStr = NULL; + VBOX_UTF16_TO_UTF8(diskSnapId, &diskSnapIdStr); + if (STREQ(diskSnapIdStr, snapshotUuidStr)) { + rc = machine->vtbl->GetStorageControllerByName(machine, + storageControllerName, +
&storageController); + VBOX_UTF16_FREE(storageControllerName); + if (!storageController) { + VBOX_RELEASE(child); + break; + } + rc = child->vtbl->GetLocation(child, &childLocUtf16); + reportInternalErrorIfNS_FAILED("cannot get disk location"); + VBOX_UTF16_TO_UTF8(childLocUtf16, &childLocUtf8); + VBOX_UTF16_FREE(childLocUtf16); + ignore_value(VIR_STRDUP(def->disks[diskCount].file, childLocUtf8)); + if (!(def->disks[diskCount].file)) { + VBOX_RELEASE(child); + VBOX_RELEASE(storageController); + virReportOOMError();
Same here - VIR_STRDUP() reports the error for you. (in the rare case that you *don't* want it automatically reported, you would call VIR_STRDUP_QUIET()).
(I probably ignored other occurrences of this problem in your patches...)
+ error = true; + break; + } + VBOX_UTF8_FREE(childLocUtf8); + + rc = storageController->vtbl->GetBus(storageController, &storageBus); + reportInternalErrorIfNS_FAILED("cannot get storage controller bus"); + rc = imediumattach->vtbl->GetType(imediumattach, &deviceType); + reportInternalErrorIfNS_FAILED("cannot get medium attachment type"); + rc = imediumattach->vtbl->GetPort(imediumattach, &devicePort); + reportInternalErrorIfNS_FAILED("cannot get medium attachchment type"); + rc = imediumattach->vtbl->GetDevice(imediumattach, &deviceSlot); + reportInternalErrorIfNS_FAILED("cannot get medium attachment device"); + def->disks[diskCount].name = vboxGenerateMediumName(storageBus, + deviceInst, + devicePort, + deviceSlot, +
maxPortPerInst, + maxSlotPerPort); + } + VBOX_UTF8_FREE(diskSnapIdStr); + } + } + VBOX_RELEASE(storageController); + VBOX_RELEASE(disk); + diskCount++; + } + vboxArrayRelease(&mediumAttachments); + /* cleanup on error */ + if (error) { + for (i = 0; i < def->dom->ndisks; i++) { + VIR_FREE(def->dom->disks[i]); + } + VIR_FREE(def->dom->disks); + def->dom->ndisks = 0; + return -1; + } + return 0; + +cleanup: + VBOX_RELEASE(snap); + return ret; +} + +static +int vboxSnapshotGetReadOnlyDisks(virDomainSnapshotPtr snapshot, + virDomainSnapshotDefPtr def) +{ + virDomainPtr dom = snapshot->domain; + VBOX_OBJECT_CHECK(dom->conn, int, -1); + vboxIID domiid = VBOX_IID_INITIALIZER; + ISnapshot *snap = NULL; + IMachine *machine = NULL; + IMachine *snapMachine = NULL; + IStorageController *storageController = NULL; + IMedium *disk = NULL; + nsresult rc; + vboxIIDFromUUID(&domiid, dom->uuid); + bool error = false; + vboxArray mediumAttachments = VBOX_ARRAY_INITIALIZER; + size_t i = 0; + PRUint32 maxPortPerInst[StorageBus_Floppy + 1] = {}; + PRUint32 maxSlotPerPort[StorageBus_Floppy + 1] = {}; + int diskCount = 0; + rc = VBOX_OBJECT_GET_MACHINE(domiid.value, &machine);
Put a blank line between the automatic data declarations and the first statement.
+ if (NS_FAILED(rc)) { + virReportError(VIR_ERR_NO_DOMAIN, "%s", + _("no domain with matching UUID")); + return -1; + }
Hmm. You defined a macro to do almost exactly this, but couldn't use it here because you couldn't goto cleanup. Are the vbox cleanup unctions/macros not safe to call when the data they're cleaning up is still NULL?
My mistake, I should have used the macro.
(not that I think you should try to use that macro - I think it should be killed. I'm just curious why you didn't use it here)
+ + if (!(snap = vboxDomainSnapshotGet(data, dom, machine, snapshot->name))) + return -1; + rc = snap->vtbl->GetMachine(snap, &snapMachine); + reportInternalErrorIfNS_FAILED("cannot get machine"); + /* + * Get READ ONLY disks + * In the snapshot metadata, these are the disks written inside the <domain> node + */ + rc = vboxArrayGet(&mediumAttachments, snapMachine, snapMachine->vtbl->GetMediumAttachments); + reportInternalErrorIfNS_FAILED("cannot get medium attachments"); + /* get the number of attachments */ + for (i = 0; i < mediumAttachments.count; i++) { + IMediumAttachment *imediumattach = mediumAttachments.items[i]; + if (imediumattach) { + IMedium *medium = NULL; + + rc = imediumattach->vtbl->GetMedium(imediumattach, &medium); + reportInternalErrorIfNS_FAILED("cannot get medium"); + if (medium) { + def->dom->ndisks++; + VBOX_RELEASE(medium); + } + } + } + + /* Allocate mem, if fails return error */ + if (VIR_ALLOC_N(def->dom->disks, def->dom->ndisks) >= 0) { + for (i = 0; i < def->dom->ndisks; i++) { + if (VIR_ALLOC(def->dom->disks[i]) < 0) { + virReportOOMError(); + error = true;
Again, unneeded call to virReportOOMError(), and it would be better to have initialized ret = -1 at the top, then done "goto cleanup" here.
+ break; + } + } + } else { + virReportOOMError(); + error = true;
Likewise. I'll stop pointing these out now...
+ } + + if (!error) + error = !vboxGetMaxPortSlotValues(data->vboxObj, maxPortPerInst, maxSlotPerPort); + + /* get the attachment details here */ + for (i = 0; i < mediumAttachments.count && diskCount < def->dom->ndisks && !error; i++) {
You' going to a lot of trouble to avoid executing this code after error has be set to true, again indicating that it would be simpler to just goto cleanup immediately when you encounter an error (and have cleanup contain the proper code to cleanup from an error).
+ PRUnichar *storageControllerName = NULL; + PRUint32 deviceType = DeviceType_Null; + PRUint32 storageBus = StorageBus_Null; + PRBool readOnly = PR_FALSE; + PRUnichar *mediumLocUtf16 = NULL; + char *mediumLocUtf8 = NULL; + PRUint32 deviceInst = 0; + PRInt32 devicePort = 0; + PRInt32 deviceSlot = 0; + IMediumAttachment *imediumattach = mediumAttachments.items[i]; + if (!imediumattach) + continue; + rc = imediumattach->vtbl->GetMedium(imediumattach, &disk); + reportInternalErrorIfNS_FAILED("cannot get medium"); + if (!disk) + continue; + rc = imediumattach->vtbl->GetController(imediumattach, &storageControllerName); + reportInternalErrorIfNS_FAILED("cannot get storage controller name"); + if (!storageControllerName) { + continue; + } + rc = machine->vtbl->GetStorageControllerByName(machine, + storageControllerName, + &storageController); + reportInternalErrorIfNS_FAILED("cannot get storage controller"); + VBOX_UTF16_FREE(storageControllerName); + if (!storageController) { + continue; + } + rc = disk->vtbl->GetLocation(disk, &mediumLocUtf16); + reportInternalErrorIfNS_FAILED("cannot get disk location"); + VBOX_UTF16_TO_UTF8(mediumLocUtf16, &mediumLocUtf8); + VBOX_UTF16_FREE(mediumLocUtf16); + ignore_value(VIR_STRDUP(def->dom->disks[diskCount]->src, mediumLocUtf8)); + + if (!(def->dom->disks[diskCount]->src)) { + virReportOOMError(); + ret = -1; + goto cleanup; + } + VBOX_UTF8_FREE(mediumLocUtf8); + + rc = storageController->vtbl->GetBus(storageController, &storageBus); + reportInternalErrorIfNS_FAILED("cannot get storage controller bus"); + if (storageBus == StorageBus_IDE) { + def->dom->disks[diskCount]->bus = VIR_DOMAIN_DISK_BUS_IDE; + } else if (storageBus == StorageBus_SATA) { + def->dom->disks[diskCount]->bus = VIR_DOMAIN_DISK_BUS_SATA; + } else if (storageBus == StorageBus_SCSI) { + def->dom->disks[diskCount]->bus = VIR_DOMAIN_DISK_BUS_SCSI; + } else if (storageBus == StorageBus_Floppy) { + def->dom->disks[diskCount]->bus = VIR_DOMAIN_DISK_BUS_FDC; + } + + rc = imediumattach->vtbl->GetType(imediumattach, &deviceType); + reportInternalErrorIfNS_FAILED("cannot get medium attachment type"); + if (deviceType == DeviceType_HardDisk) + def->dom->disks[diskCount]->device = VIR_DOMAIN_DISK_DEVICE_DISK; + else if (deviceType == DeviceType_Floppy) + def->dom->disks[diskCount]->device = VIR_DOMAIN_DISK_DEVICE_FLOPPY; + else if (deviceType == DeviceType_DVD) + def->dom->disks[diskCount]->device = VIR_DOMAIN_DISK_DEVICE_CDROM; + + rc = imediumattach->vtbl->GetPort(imediumattach, &devicePort); + reportInternalErrorIfNS_FAILED("cannot get medium attachment port"); + rc = imediumattach->vtbl->GetDevice(imediumattach, &deviceSlot); + reportInternalErrorIfNS_FAILED("cannot get device"); + rc = disk->vtbl->GetReadOnly(disk, &readOnly); + reportInternalErrorIfNS_FAILED("cannot get read only attribute"); + if (readOnly == PR_TRUE) + def->dom->disks[diskCount]->readonly = true; + def->dom->disks[diskCount]->type = VIR_DOMAIN_DISK_TYPE_FILE; + def->dom->disks[diskCount]->dst = vboxGenerateMediumName(storageBus, + deviceInst, + devicePort, + deviceSlot, + maxPortPerInst, + maxSlotPerPort); + if (!def->dom->disks[diskCount]->dst) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Could not generate medium name for the disk " + "at: controller instance:%u, port:%d, slot:%d"), + deviceInst, devicePort, deviceSlot); + ret = -1; + goto cleanup; + } + diskCount ++; + } + /* cleanup on error */
... but not on *all* errors, since you sometimes set error = true and continue all the way down to here, and sometime you set set = -1 and goto cleanup (thus skipping this extra cleanup). ALL of the cleanup should be after the cleanup label, and you should always goto cleanup when you encounter an error. That way you won't have some error recovery paths that skip some of the cleanup.
+ if (error) { + for (i = 0; i < def->dom->ndisks; i++) { + VIR_FREE(def->dom->disks[i]); + } + VIR_FREE(def->dom->disks); + def->dom->ndisks = 0; + ret = -1; + goto cleanup; + } + ret = 0; +cleanup: + VBOX_RELEASE(disk); + VBOX_RELEASE(storageController); + vboxArrayRelease(&mediumAttachments); + VBOX_RELEASE(snap); + return ret; +} +#endif +
static char * vboxDomainSnapshotGetXMLDesc(virDomainSnapshotPtr snapshot,
unsigned int flags)
@@ -6065,6 +6417,10 @@ vboxDomainSnapshotGetXMLDesc(virDomainSnapshotPtr snapshot,
PRInt64 timestamp; PRBool online = PR_FALSE; char uuidstr[VIR_UUID_STRING_BUFLEN];
+#if VBOX_API_VERSION >=4002 + PRUint32 memorySize = 0; + PRUint32 CPUCount = 0; +#endif
virCheckFlags(0, NULL);
@@ -6079,11 +6435,40 @@ vboxDomainSnapshotGetXMLDesc(virDomainSnapshotPtr snapshot,
if (!(snap = vboxDomainSnapshotGet(data, dom, machine, snapshot->name)))
goto cleanup;
- if (VIR_ALLOC(def) < 0) + if (VIR_ALLOC(def) < 0 + || VIR_ALLOC(def->dom) < 0)
goto cleanup;
if (VIR_STRDUP(def->name, snapshot->name) < 0)
goto cleanup;
+#if VBOX_API_VERSION >=4002 + /* Register def->dom properties for them to be saved inside the snapshot XMl + * Otherwise, there is a problem while parsing the xml + */ + def->dom->virtType = VIR_DOMAIN_VIRT_VBOX; + def->dom->id = dom->id; + memcpy(def->dom->uuid, dom->uuid, VIR_UUID_BUFLEN); + ignore_value(VIR_STRDUP(def->dom->name, dom->name)); + machine->vtbl->GetMemorySize(machine, &memorySize); + def->dom->mem.cur_balloon = memorySize * 1024; + /* Currently setting memory and maxMemory as same, cause + * the notation here seems to be inconsistent while + * reading and while dumping xml + */ + def->dom->mem.max_balloon = memorySize * 1024; + ignore_value(VIR_STRDUP(def->dom->os.type, "hvm")); + def->dom->os.arch = virArchFromHost(); + machine->vtbl->GetCPUCount(machine, &CPUCount); + def->dom->maxvcpus = def->dom->vcpus = CPUCount; + if (vboxSnapshotGetReadWriteDisks(def, snapshot) < 0) { + VIR_DEBUG("Could not get read write disks for snapshot"); + } + + if (vboxSnapshotGetReadOnlyDisks(snapshot, def) <0) { + VIR_DEBUG("Could not get Readonly disks for snapshot"); + } +#endif /* VBOX_API_VERSION >= 4002 */ +
rc = snap->vtbl->GetDescription(snap, &str16); if (NS_FAILED(rc)) {
virReportError(VIR_ERR_INTERNAL_ERROR,
@@ -6148,8 +6533,8 @@ vboxDomainSnapshotGetXMLDesc(virDomainSnapshotPtr snapshot,
def->state = VIR_DOMAIN_SHUTOFF;
virUUIDFormat(dom->uuid, uuidstr);
+ memcpy(def->dom->uuid, dom->uuid, VIR_UUID_BUFLEN);
ret = virDomainSnapshotDefFormat(uuidstr, def, flags, 0);
-
cleanup: virDomainSnapshotDefFree(def); VBOX_RELEASE(parent);
@@ -6854,6 +7239,8 @@ cleanup: return ret;
}
+ +
static int vboxDomainSnapshotDelete(virDomainSnapshotPtr snapshot,
unsigned int flags)
@@ -6889,8 +7276,9 @@ vboxDomainSnapshotDelete(virDomainSnapshotPtr snapshot,
goto cleanup;
}
- /* VBOX snapshots do not require any libvirt metadata, making this - * flag trivial once we know we have a valid snapshot. */ + /* In case we just want to delete the metadata, we will edit the vbox file in order + *to remove the node concerning the snapshot + */
if (flags & VIR_DOMAIN_SNAPSHOT_DELETE_METADATA_ONLY) {
ret = 0; goto cleanup;
@@ -8716,8 +9104,9 @@ cleanup: return ret;
}
-static int vboxStorageVolDelete(virStorageVolPtr vol, - unsigned int flags) +static int vboxStorageDeleteOrClose(virStorageVolPtr vol, + unsigned int flags, + unsigned int flagDeleteOrClose)
{
VBOX_OBJECT_CHECK(vol->conn, int, -1); vboxIID hddIID = VBOX_IID_INITIALIZER;
@@ -8872,8 +9261,18 @@ static int vboxStorageVolDelete(virStorageVolPtr vol,
if (machineIdsSize == 0 || machineIdsSize == deregister) {
IProgress *progress = NULL;
- +#if VBOX_API_VERSION >= 4002 + if (flagDeleteOrClose & VBOX_STORAGE_CLOSE_FLAG) { + rc = hardDisk->vtbl->Close(hardDisk); + if (NS_SUCCEEDED(rc)) { + DEBUGIID("HardDisk closed, UUID", hddIID.value); + ret = 0; + } + } +#endif + if (flagDeleteOrClose & VBOX_STORAGE_DELETE_FLAG){
rc = hardDisk->vtbl->DeleteStorage(hardDisk, &progress);
+ }
if (NS_SUCCEEDED(rc) && progress) {
progress->vtbl->WaitForCompletion(progress, -1);
@@ -8892,6 +9291,12 @@ static int vboxStorageVolDelete(virStorageVolPtr vol,
return ret;
}
+static int vboxStorageVolDelete(virStorageVolPtr vol, + unsigned int flags) +{ + return vboxStorageDeleteOrClose(vol, flags, VBOX_STORAGE_DELETE_FLAG); +} +
static int vboxStorageVolGetInfo(virStorageVolPtr vol, virStorageVolInfoPtr info) {
VBOX_OBJECT_CHECK(vol->conn, int, -1); IHardDisk *hardDisk = NULL;
On 12/23/2013 05:48 PM, Manuel VIVES wrote:
@@ -273,10 +276,16 @@ static vboxGlobalData *g_pVBoxGlobalData = NULL;
#endif /* VBOX_API_VERSION >= 4000 */
+#define reportInternalErrorIfNS_FAILED(message) \ + if (NS_FAILED(rc)) { \ + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _(message)); \ + goto cleanup; \ + } + + I would prefer these if() clauses with goto to be explicitly in the code, rather than hidden in a macro. That way it's easier to see all
On 11/25/2013 07:23 PM, Manuel VIVES wrote: potential code paths when scanning through the code. If you have a goto hidden in a macro, then it's not immediately visible and that can only lead to problems. I removed this macro in order to have 'goto' instructions clearly visible but it increases the size of the patch.
That's fine. I think the extra few lines is worth the clarity.
The snapshots are saved in xml files, and then can be redefined --- src/vbox/vbox_tmpl.c | 852 +++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 844 insertions(+), 8 deletions(-) diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c index e05ed97..23f8aab 100644 --- a/src/vbox/vbox_tmpl.c +++ b/src/vbox/vbox_tmpl.c @@ -61,6 +61,7 @@ #include "virstring.h" #include "virtime.h" #include "virutil.h" +#include "dirname.h" /* This one changes from version to version. */ #if VBOX_API_VERSION == 2002 @@ -276,6 +277,12 @@ static vboxGlobalData *g_pVBoxGlobalData = NULL; #endif /* VBOX_API_VERSION >= 4000 */ +/*This error is a bit specific + *In the VBOX API it is named E_ACCESSDENIED + *It is returned when the called object is not ready. In + *particular when we do any call on a disk which has been closed +*/ +#define VBOX_E_ACCESSDENIED 0x80070005 #define reportInternalErrorIfNS_FAILED(message) \ if (NS_FAILED(rc)) { \ virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _(message)); \ @@ -286,6 +293,8 @@ static vboxGlobalData *g_pVBoxGlobalData = NULL; static virDomainPtr vboxDomainDefineXML(virConnectPtr conn, const char *xml); static int vboxDomainCreate(virDomainPtr dom); static int vboxDomainUndefineFlags(virDomainPtr dom, unsigned int flags); +static virStorageVolPtr vboxStorageVolLookupByPath(virConnectPtr conn, const char *path); +static int vboxStorageDeleteOrClose(virStorageVolPtr vol, unsigned int flags, unsigned int flagDeleteOrClose); static void vboxDriverLock(vboxGlobalData *data) { virMutexLock(&data->lock); } @@ -5946,6 +5955,827 @@ cleanup: return snapshot; } +#if VBOX_API_VERSION >=4002 +static void +vboxSnapshotXmlRetrieveSnapshotNodeByName(xmlNodePtr a_node, + const char *name, + xmlNodePtr *snap_node) +{ + xmlNodePtr cur_node = NULL; + + for (cur_node = a_node; cur_node; cur_node = cur_node->next) { + if (cur_node->type == XML_ELEMENT_NODE) { + if (!xmlStrcmp(cur_node->name, (const xmlChar *) "Snapshot") && + STREQ(virXMLPropString(cur_node, "name"), name)) { + *snap_node = cur_node; + return; + } + } + if (cur_node->children) + vboxSnapshotXmlRetrieveSnapshotNodeByName(cur_node->children, name, snap_node); + } +} + + + + +static int +vboxDetachAndCloseDisks(virDomainPtr dom, + IMedium *disk) +{ + VBOX_OBJECT_CHECK(dom->conn, int, -1); + nsresult rc; + PRUnichar *location = NULL; + vboxArray childrenDiskArray = VBOX_ARRAY_INITIALIZER; + virStorageVolPtr volPtr = NULL; + char *location_utf8 = NULL; + PRUint32 dummyState = 0; + size_t i = 0; + if (disk == NULL) { + VIR_DEBUG("Null pointer to disk"); + return -1; + } + rc = disk->vtbl->GetLocation(disk, &location); + if (rc == VBOX_E_ACCESSDENIED) { + VIR_DEBUG("Disk already closed"); + goto cleanup; + } + reportInternalErrorIfNS_FAILED("cannot get disk location"); + rc = vboxArrayGet(&childrenDiskArray, disk, disk->vtbl->GetChildren); + reportInternalErrorIfNS_FAILED("cannot get children disks"); + for (i = 0; i < childrenDiskArray.count; ++i) { + IMedium *childDisk = childrenDiskArray.items[i]; + if (childDisk) { + vboxDetachAndCloseDisks(dom, childDisk); + } + } + rc = disk->vtbl->RefreshState(disk, &dummyState); + reportInternalErrorIfNS_FAILED("cannot refresh state"); + VBOX_UTF16_TO_UTF8(location, &location_utf8); + volPtr = vboxStorageVolLookupByPath(dom->conn, location_utf8); + + if (volPtr) { + VIR_DEBUG("Closing %s", location_utf8); + if (vboxStorageDeleteOrClose(volPtr, 0, VBOX_STORAGE_CLOSE_FLAG) != 0) { + VIR_DEBUG("Error while closing disk"); + } + } + VBOX_UTF8_FREE(location_utf8); +cleanup: + VBOX_UTF16_FREE(location); + vboxArrayRelease(&childrenDiskArray); + return ret; +} + +static void +vboxSnapshotXmlAddChild(xmlNodePtr parent, + xmlNodePtr child) +{ + /*Used in order to add child without writing the stuff concerning xml namespaces*/ + xmlBufferPtr tmpBuf = xmlBufferCreate(); + char *tmpString = NULL; + xmlNodePtr tmpNode = NULL; + xmlNodeDump(tmpBuf, parent->doc, child, 0, 0); + ignore_value(VIR_STRDUP(tmpString, (char *)xmlBufferContent(tmpBuf))); + xmlParseInNodeContext(parent, tmpString, (int)strlen(tmpString), 0, &tmpNode); + if (tmpNode) { + if (xmlAddChild(parent, xmlCopyNode(tmpNode, 1)) == NULL) { + VIR_DEBUG("Error while adding %s to %s", (char *)tmpNode->name, (char *)parent->name); + } + } + xmlFree(tmpNode); + xmlBufferFree(tmpBuf); +} + +static void +vboxSnapshotXmlRetrieveMachineNode(xmlNodePtr root, + xmlNodePtr *machineNode) +{ + xmlNodePtr cur = root->xmlChildrenNode; + while (cur && xmlIsBlankNode(cur)) { + cur = cur -> next; + } + if (xmlStrcmp(cur->name, (const xmlChar *) "Machine")) { + VIR_DEBUG("Problem, found %s, Machine expected", (char *)cur->name); + return; + } + *machineNode = cur; +} + +static void +vboxSnapshotXmlRetrieveSnapshotsNode(xmlNodePtr snapshotNode, + xmlNodePtr *snapshotsNode) +{ + xmlNodePtr cur_node = NULL; + + for (cur_node = snapshotNode; cur_node; cur_node = cur_node->next) { + if (cur_node->type == XML_ELEMENT_NODE) { + if (!xmlStrcmp(cur_node->name, (const xmlChar *) "Snapshots")) { + *snapshotsNode = cur_node; + return; + } + } + if (cur_node->children) + vboxSnapshotXmlRetrieveSnapshotsNode(cur_node->children, snapshotsNode); + } +} + +static void +vboxSnapshotXmlRetrieveRootNodeByName(xmlNodePtr root, + const char *name, + xmlNodePtr *returnNode) +{ + xmlNodePtr cur = root->xmlChildrenNode; + while (cur && xmlIsBlankNode(cur)) { + cur = cur -> next; + } + if (xmlStrcmp(cur->name, (const xmlChar *) "Machine")) { + VIR_DEBUG("Problem, found %s, Machine expected", (char *)cur->name); + } + cur=cur->xmlChildrenNode; + while (cur != NULL) { + if (!xmlStrcmp(cur->name, (const xmlChar*) name)) { + *returnNode = cur; + return; + } + cur = cur -> next; + } +} + +static void +vboxSnapshotXmlAppendDiskToMediaRegistry(xmlNodePtr *inMediaRegistry, + char *parentId, + char *childId, + char *childLocation, + char *childFormat) +{ + /*This function will modify the inMediaregistry node to append all the informations about the child disk + */ + xmlNodePtr cur_node = NULL; + for (cur_node = *inMediaRegistry; cur_node; cur_node = cur_node->next) { + if (cur_node) { + if (cur_node->type == XML_ELEMENT_NODE + && !xmlStrcmp(cur_node->name, (const xmlChar *) "HardDisk") + && xmlHasProp(cur_node, BAD_CAST "uuid") != NULL + && STREQ((char *)xmlHasProp(cur_node, BAD_CAST "uuid")->children->content, parentId)) { + + xmlNodePtr childDiskNode = xmlNewTextChild(cur_node, NULL, BAD_CAST "HardDisk", NULL); + xmlNewProp(childDiskNode, BAD_CAST "uuid", BAD_CAST childId); + xmlNewProp(childDiskNode, BAD_CAST "location", BAD_CAST childLocation); + xmlNewProp(childDiskNode, BAD_CAST "format", BAD_CAST childFormat); + return; + } + } + if (&(cur_node->children)) + vboxSnapshotXmlAppendDiskToMediaRegistry(&(cur_node->children), parentId, childId, childLocation, childFormat); + + } +} + +static int +vboxSnapshotGenerateVboxXML(xmlNodePtr rootElementVboxXML, + char *storageControllerString, + char *parent, + char *defName, + char *timeStamp, + char *uuid, + xmlNodePtr *snapshotNodeReturned) +{ + xmlDocPtr doc; + xmlNodePtr snapshotNode; + xmlNodePtr snapshotsNode; + xmlNodePtr machineHardwareNode = NULL; + char *uuidstrWithBrackets = NULL; + char *timeVboxFormat = NULL; + int ret = -1; + /*We change the date format from "yyyy-MM-dd hh:mm:ss.msec+timeZone" to "yyyy-MM-ddThh:mm:ssZ"*/ + char *date = virStringSplit(virStringJoin((const char**)virStringSplit(virStringSplit(timeStamp, "+", 0)[0], " ", 0), "T"), ".", 0)[0]; + + /*Retrieve hardware*/ + vboxSnapshotXmlRetrieveRootNodeByName(rootElementVboxXML, "Hardware", &machineHardwareNode); + /* Create a new XML DOM tree, to which the XML document will be + * written */ + doc = xmlNewDoc(BAD_CAST XML_DEFAULT_VERSION); + if (doc == NULL) { + VIR_DEBUG("Error creating the xml document tree\n"); + ret = -1; + goto cleanup; + } + /* Create a new XML node, to which the XML document will be + * appended */ + snapshotNode = xmlNewDocNode(doc, NULL, BAD_CAST "Snapshot", NULL); + if (snapshotNode == NULL) { + VIR_DEBUG("Error creating the xml node Snapshot\n"); + ret = -1; + goto cleanup; + } + if (virAsprintf(&uuidstrWithBrackets, "{%s}", uuid) != -1) { + xmlNewProp(snapshotNode, BAD_CAST "uuid", BAD_CAST uuidstrWithBrackets); + VIR_FREE(uuidstrWithBrackets); + } + xmlNewProp(snapshotNode, BAD_CAST "name", BAD_CAST defName); + if (virAsprintf(&timeVboxFormat, "%sZ", date) != -1) { + xmlNewProp(snapshotNode, BAD_CAST "timeStamp", BAD_CAST timeVboxFormat); + VIR_FREE(timeVboxFormat); + } + xmlDocSetRootElement(doc, snapshotNode); + if (machineHardwareNode) { + vboxSnapshotXmlAddChild(snapshotNode, machineHardwareNode); + } + xmlNodePtr listStorageControllerNodes; + xmlParseInNodeContext(snapshotNode, storageControllerString, (int)strlen(storageControllerString), 0, &listStorageControllerNodes); + if (listStorageControllerNodes) { + if (xmlAddChild(snapshotNode, xmlCopyNode(listStorageControllerNodes, 1)) == NULL) { + VIR_DEBUG("Could not add listStorageControllerNodes node to snapshot node"); + ret = -1; + goto cleanup; + } + } + snapshotsNode = xmlNewDocNode(doc, NULL, BAD_CAST "Snapshots", NULL); + if (snapshotsNode == NULL) { + VIR_DEBUG("Error creating the xml node Snapshots\n"); + ret = -1; + goto cleanup; + } + if (snapshotsNode) { + if (xmlAddChild(snapshotNode, xmlCopyNode(snapshotsNode, 1)) == NULL) { + VIR_DEBUG("Could not add snapshotsNode node to snapshot node"); + ret = -1; + goto cleanup; + } + } + if (parent) { + xmlNodePtr rootSnapshotNode = NULL; + xmlNodePtr parentNode = NULL; + xmlNodePtr parentSnapshotsNode = NULL; + vboxSnapshotXmlRetrieveRootNodeByName(rootElementVboxXML, "Snapshot", &rootSnapshotNode); + if (!rootSnapshotNode) { + VIR_DEBUG("Could not retrieve Snapshot node"); + ret = -1; + goto cleanup; + } + vboxSnapshotXmlRetrieveSnapshotNodeByName(rootSnapshotNode, parent, &parentNode); + if (!parentNode) { + VIR_DEBUG("Could not retrieve Snapshot node of %s", parent); + ret = -1; + goto cleanup; + } + vboxSnapshotXmlRetrieveSnapshotsNode(parentNode->children, &parentSnapshotsNode); + if (!parentSnapshotsNode) { + VIR_DEBUG("Could not retrieve Snapshots node"); + ret = -1; + goto cleanup; + } + if (xmlAddChild(parentSnapshotsNode, xmlCopyNode(snapshotNode, 1)) == NULL) { + VIR_DEBUG("Could not add snapshotsNode node to its parent"); + ret = -1; + goto cleanup; + } else { + *snapshotNodeReturned = xmlCopyNode(rootSnapshotNode, 1); + } + } else { + *snapshotNodeReturned = xmlCopyNode(snapshotNode, 1); + } + ret = 0; +cleanup: + return ret; +} + +static xmlBufferPtr +vboxSnapshotGenerateXmlSkeleton(char *domainUuid, + char *machineName, + char *snapshotUuid, + char *snapshotFolder, + PRInt64 lastStateChange, + bool isCurrent, + char *savedCurrentSnapshotUuid) +{ + char *snapshotUuidWithBrackets = NULL; + char *lastStateChangeStr = NULL; + /*Let's write a new xml + */ + xmlTextWriterPtr writer; + xmlBufferPtr buf = xmlBufferCreate(); + xmlBufferPtr ret = NULL; + int resultXml; + char *uuidWithBracket = NULL; + + /* Create a new XmlWriter with no compression. */ + writer = xmlNewTextWriterMemory(buf, 0); + if (writer == NULL) { + VIR_DEBUG("Error creating the xml writer\n"); + goto cleanup; + } + + /* Start the document with the xml default for the version, + * encoding ISO 8859-1 and the default for the standalone + * declaration. */ + resultXml = xmlTextWriterStartDocument(writer, NULL, "ISO-8859-1", NULL); + if (resultXml < 0) { + VIR_DEBUG("Error at xmlTextWriterStartDocument\n"); + goto cleanup; + } + /* Write a comment as child of root. + * Please observe, that the input to the xmlTextWriter functions + * HAS to be in UTF-8, even if the output XML is encoded + * in iso-8859-1 */ + resultXml = xmlTextWriterWriteFormatComment(writer, "WARNING: THIS IS AN AUTO-GENERATED FILE. CHANGES TO IT ARE LIKELY TO BE \n" + "OVERWRITTEN AND LOST.\n" + "Changes to this xml configuration should be made using Virtualbox \n" + "or other application using the libvirt API"); + if (resultXml < 0) { + VIR_DEBUG("Error at xmlTextWriterWriteComment\n"); + goto cleanup; + } + xmlTextWriterWriteRaw(writer, BAD_CAST "\n"); /*Break line after comment*/ + + /* Start an element named "VirtualBox". Since thist is the first + * element, this will be the root element of the document. */ + resultXml = xmlTextWriterStartElement(writer, BAD_CAST "VirtualBox"); + if (resultXml < 0) { + VIR_DEBUG("Error creating the xml node\n"); + goto cleanup; + } + resultXml = xmlTextWriterWriteAttribute(writer, BAD_CAST "version", BAD_CAST "1.12-linux"); + if (resultXml < 0) { + VIR_DEBUG("Error at xmlTextWriterWriteAttribute\n"); + goto cleanup; + } + /* Start an element named "Machine" as child of VirtualBox. */ + resultXml = xmlTextWriterStartElement(writer, BAD_CAST "Machine"); + if (resultXml < 0) { + VIR_DEBUG("Error at xmlTextWriterStartElement\n"); + goto cleanup; + } + /* Add an attribute with name "uuid" and value "{uuid}" to Machine. */ + if (virAsprintf(&uuidWithBracket, "{%s}", domainUuid) != -1) { + resultXml = xmlTextWriterWriteAttribute(writer, BAD_CAST "uuid", BAD_CAST uuidWithBracket); + if (resultXml < 0) { + VIR_DEBUG("Error at xmlTextWriterWriteAttribute\n"); + goto cleanup; + } + VIR_FREE(uuidWithBracket); + } + + if (!machineName) { + VIR_DEBUG("No machine name"); + goto cleanup; + } + resultXml = xmlTextWriterWriteAttribute(writer, BAD_CAST "name", BAD_CAST machineName); + if (resultXml < 0) { + VIR_DEBUG("Error at xmlTextWriterWriteAttribute\n"); + goto cleanup; + } + resultXml = xmlTextWriterWriteAttribute(writer, BAD_CAST "OSType", BAD_CAST "Other"); + if (resultXml < 0) { + VIR_DEBUG("Error at xmlTextWriterWriteAttribute\n"); + goto cleanup; + } + + if (!snapshotFolder) { + VIR_DEBUG("No machine snapshotFolder"); + goto cleanup; + } + resultXml = xmlTextWriterWriteAttribute(writer, BAD_CAST "snapshotFolder", BAD_CAST snapshotFolder); + if (resultXml < 0) { + VIR_DEBUG("Error at xmlTextWriterWriteAttribute\n"); + goto cleanup; + } + if (virAsprintf(&lastStateChangeStr, "%ld", lastStateChange) < 0) { + VIR_DEBUG("Could not convert lastStateChange from long to string"); + goto cleanup; + } + resultXml = xmlTextWriterWriteAttribute(writer, BAD_CAST "lastStateChange", BAD_CAST lastStateChangeStr); + if (resultXml < 0) { + VIR_DEBUG("Error at xmlTextWriterWriteAttribute\n"); + goto cleanup; + } + /* Current Snapshot + * https://www.virtualbox.org/sdkref/interface_i_machine.html#ac785dbe04eccc079... + * There is always a current snapshot, except when there is no snapshot (obvious) or when they have all been deleted (obvious) + * or when the current snapshot is deleted and it does not have a parent (not so obvious). + * This can happen only when the last snapshot is deleted because there can only be one snapshot with no parent, the first one. + * For the moment we always put the snapshot we are defining as current snapshot. When all the snapshots are redefined, you can always set + * the current snapshot to one you have previously saved. + */ + if (isCurrent) { + if (virAsprintf(&snapshotUuidWithBrackets, "{%s}", snapshotUuid) != -1) { + resultXml = xmlTextWriterWriteAttribute(writer, BAD_CAST "currentSnapshot", BAD_CAST snapshotUuidWithBrackets); + if (resultXml < 0) { + VIR_DEBUG("Error at xmlTextWriterWriteAttribute\n"); + goto cleanup; + } + VIR_FREE(snapshotUuidWithBrackets); + } + } + if (savedCurrentSnapshotUuid != NULL) { + if (virAsprintf(&snapshotUuidWithBrackets, "{%s}", savedCurrentSnapshotUuid) != -1) { + resultXml = xmlTextWriterWriteAttribute(writer, BAD_CAST "currentSnapshot", BAD_CAST snapshotUuidWithBrackets); + if (resultXml < 0) { + VIR_DEBUG("Error at xmlTextWriterWriteAttribute\n"); + goto cleanup; + } + VIR_FREE(snapshotUuidWithBrackets); + } + } + + /* Here we could close the elements Machine and VirtualBox using the + * function xmlTextWriterEndElement, but since we do not want to + * write any other elements, we simply call xmlTextWriterEndDocument, + * which will do all the work. */ + resultXml = xmlTextWriterEndDocument(writer); + if (resultXml < 0) { + VIR_DEBUG("Error at xmlTextWriterEndDocument\n"); + goto cleanup; + } + xmlFreeTextWriter(writer); + /* Now we have a file containing the skeleton for the xml we want to build + * It should look like this: + * <?xml version="1.0" encoding="ISO-8859-1"?> + * <!--comment + * --> + * <VirtualBox> + * <Machine uuid="{anUuid}" name="the name" OSType="Other" snapshotFolder="/path/to/SnapshotFolder" lastStateChange="a date"/> + * </VirtualBox> + */ + ret = buf; +cleanup: + return ret; +} + +static int +vboxSnapshotRedefine(virDomainPtr dom, + virDomainSnapshotDefPtr def, + bool isCurrent) +{ + /* + * If your snapshot has a parent, + * it will only be redefined if you have already + * redefined the parent. + * + */ + + VBOX_OBJECT_CHECK(dom->conn, int, 0); + vboxIID domiid = VBOX_IID_INITIALIZER; + IMachine *machine = NULL; + nsresult rc; + PRUnichar *settingsFilePath = NULL; + char *settingsFilePath_Utf8 = NULL; + size_t it = 0; + + IMachine *newMachine = NULL; + xmlDocPtr xml = NULL; + bool snapshotHasParents = !(def->parent == NULL); + + vboxIIDFromUUID(&domiid, dom->uuid); + rc = VBOX_OBJECT_GET_MACHINE(domiid.value, &machine); + if (NS_FAILED(rc)) { + virReportError(VIR_ERR_NO_DOMAIN, "%s", + _("no domain with matching UUID")); + ret = -1; + goto cleanup; + } + + + rc = machine->vtbl->SaveSettings(machine); + rc = machine->vtbl->GetSettingsFilePath(machine, &settingsFilePath); + reportInternalErrorIfNS_FAILED("cannot get settings file path"); + VBOX_UTF16_TO_UTF8(settingsFilePath, &settingsFilePath_Utf8); + xml = virXMLParse(settingsFilePath_Utf8, NULL, NULL); + if (xml) { + xmlNodePtr cur = NULL; + xmlNodePtr machineHardwareNode = NULL; + xmlNodePtr extraDataNode = NULL; + xmlNodePtr mediaRegistryNodeOriginal = NULL; + xmlNodePtr snapshotStorageControllersNode = NULL; + xmlNodePtr modifiedMachineStorageControllersNode = NULL; + char *newMachineStorageControllersString = NULL; + char *newSnapshotStorageControllersString = NULL; + char snapshotuuidstr[VIR_UUID_STRING_BUFLEN]; + unsigned char snapshotUuid[VIR_UUID_BUFLEN]; + + char *domainUuid = NULL; + PRUnichar *machineNameUtf16 = NULL; + char *machineName = NULL; + PRUnichar *snapshotFolderUtf16 = NULL; + PRInt64 lastStateChange = 0; + ISnapshot *savedCurrentSnapshot = NULL; + vboxIID savedCurrentSnapshotIid = VBOX_IID_INITIALIZER; + char *savedCurrentSnapshotUuid = NULL; + cur = xmlDocGetRootElement(xml); + xmlBufferPtr buf = xmlBufferCreate(); + if (cur == NULL) { + VIR_DEBUG("empty document\n"); + ret = -1; + goto cleanup; + } + /*Retrieve hardware*/ + vboxSnapshotXmlRetrieveRootNodeByName(cur, "Hardware", &machineHardwareNode); + if (!machineHardwareNode) { + VIR_DEBUG("Could not retrieve Hardware node"); + ret = -1; + goto cleanup; + } + /*Retrieve storageController*/ + buf = xmlBufferCreate(); + vboxSnapshotXmlRetrieveRootNodeByName(cur, "StorageControllers", &snapshotStorageControllersNode); + if (!snapshotStorageControllersNode) { + VIR_DEBUG("Could not retrieve StorageControllers node"); + ret = -1; + goto cleanup; + } + xmlNodeDump(buf, xml, snapshotStorageControllersNode, 0, 0); + ignore_value(VIR_STRDUP(newMachineStorageControllersString, (char *)xmlBufferContent(buf))); + ignore_value(VIR_STRDUP(newSnapshotStorageControllersString, newMachineStorageControllersString)); + xmlBufferFree(buf); + /*Retrieve ExtraData*/ + vboxSnapshotXmlRetrieveRootNodeByName(cur, "ExtraData", &extraDataNode); + if (!extraDataNode) { + VIR_DEBUG("Could not retrieve ExtraData node"); + ret = -1; + goto cleanup; + } + /*Retrieve MediaRegistry*/ + vboxSnapshotXmlRetrieveRootNodeByName(cur, "MediaRegistry", &mediaRegistryNodeOriginal); + if (!mediaRegistryNodeOriginal) { + VIR_DEBUG("Could not retrieve MediaRegistry node"); + ret = -1; + goto cleanup; + } + vboxArray mediumAttachments = VBOX_ARRAY_INITIALIZER; + rc = vboxArrayGet(&mediumAttachments, machine, machine->vtbl->GetMediumAttachments); + reportInternalErrorIfNS_FAILED("cannot get medium attachments"); + + /*Read only disks*/ + for (it = 0; it<def->dom->ndisks; ++it) { + IMedium *mediumReadOnly = NULL; + PRUnichar *locationReadOnlyDiskUtf16 = NULL; + PRUnichar *mediumReadOnlyFormatUtf16 = NULL; + PRUnichar *mediumReadOnlyIdUtf16 = NULL; + char *mediumReadOnlyFormat = NULL; + char *mediumReadOnlyId = NULL; + char *mediaRegistryNodeOriginalString = NULL; + VBOX_UTF8_TO_UTF16(def->dom->disks[it]->src, &locationReadOnlyDiskUtf16); + data->vboxObj->vtbl->OpenMedium(data->vboxObj, locationReadOnlyDiskUtf16, DeviceType_HardDisk, AccessMode_ReadWrite, false, &mediumReadOnly); + if (mediumReadOnly) { + rc = mediumReadOnly->vtbl->GetId(mediumReadOnly, &mediumReadOnlyIdUtf16); + reportInternalErrorIfNS_FAILED("cannot get medium id"); + rc = mediumReadOnly->vtbl->GetFormat(mediumReadOnly, &mediumReadOnlyFormatUtf16); + reportInternalErrorIfNS_FAILED("cannot get medium format"); + VBOX_UTF16_TO_UTF8(mediumReadOnlyIdUtf16, &mediumReadOnlyId); + VBOX_UTF16_TO_UTF8(mediumReadOnlyFormatUtf16, &mediumReadOnlyFormat); + char *uuidToReplace = NULL; + virSearchUuid(newSnapshotStorageControllersString, it+1, &uuidToReplace); + if (STRNEQ(uuidToReplace, mediumReadOnlyId)) { + char *tmp = NULL; + tmp = virStrReplace(newSnapshotStorageControllersString, uuidToReplace, mediumReadOnlyId); + VIR_FREE(newSnapshotStorageControllersString); + if (VIR_STRDUP(newSnapshotStorageControllersString, tmp) < 0) { + VIR_DEBUG("Error while using strdup"); + } + VIR_FREE(tmp); + } + IMedium *mediumReadOnlyParent = NULL; + rc = mediumReadOnly->vtbl->GetParent(mediumReadOnly, &mediumReadOnlyParent); + reportInternalErrorIfNS_FAILED("cannot get medium parent"); + if (mediumReadOnlyParent) { + PRUnichar *parentIdUtf16 = NULL; + char *parentId = NULL; + char *parentIdWithBrackets = NULL; + char *childIdWithBrackets = NULL; + rc = mediumReadOnlyParent->vtbl->GetId(mediumReadOnlyParent, &parentIdUtf16); + reportInternalErrorIfNS_FAILED("cannot get medium id"); + if (parentIdUtf16) { + VBOX_UTF16_TO_UTF8(parentIdUtf16, &parentId); + if (virAsprintf(&parentIdWithBrackets, "{%s}", parentId) != -1 && virAsprintf(&childIdWithBrackets, "{%s}", mediumReadOnlyId) != -1) { + buf = xmlBufferCreate(); + xmlNodeDump(buf, mediaRegistryNodeOriginal->doc, mediaRegistryNodeOriginal, 0, 0); + ignore_value(VIR_STRDUP(mediaRegistryNodeOriginalString, (char *)xmlBufferContent(buf))); + if (strstr(mediaRegistryNodeOriginalString, childIdWithBrackets) == NULL) { + vboxSnapshotXmlAppendDiskToMediaRegistry(&mediaRegistryNodeOriginal->children, parentIdWithBrackets, childIdWithBrackets, def->dom->disks[it]->src, mediumReadOnlyFormat); + } else { + VIR_DEBUG("Already added"); + } + VIR_FREE(parentIdWithBrackets); + VIR_FREE(childIdWithBrackets); + } + VBOX_UTF8_FREE(parentId); + } + VBOX_RELEASE(mediumReadOnlyParent); + VBOX_UTF16_FREE(parentIdUtf16); + } + VBOX_UTF8_FREE(mediumReadOnlyFormat); + VBOX_UTF8_FREE(mediumReadOnlyId); + VBOX_RELEASE(mediumReadOnly); + } + VBOX_UTF16_FREE(mediumReadOnlyIdUtf16); + VBOX_UTF16_FREE(mediumReadOnlyFormatUtf16); + VBOX_UTF16_FREE(locationReadOnlyDiskUtf16); + } + + /*Read Write disks*/ + for (it = 0; it< def->ndisks; ++it) { + IMedium *mediumReadWrite = NULL; + PRUnichar *mediumReadWriteFormatUtf16 = NULL; + PRUnichar *mediumReadWriteIdUtf16 = NULL; + PRUnichar *locationReadWriteDiskUtf16 = NULL; + char *mediumReadWriteFormat = NULL; + char *mediumReadWriteId = NULL; + char *mediaRegistryNodeOriginalString = NULL; + VBOX_UTF8_TO_UTF16(def->disks[it].file, &locationReadWriteDiskUtf16); + rc = data->vboxObj->vtbl->OpenMedium(data->vboxObj, locationReadWriteDiskUtf16, DeviceType_HardDisk, AccessMode_ReadWrite, false, &mediumReadWrite); + if (NS_FAILED(rc)) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s" + "%s", + _("Could not open medium"), def->disks[it].file); + ret = -1; + goto cleanup; + } + if (mediumReadWrite) { + rc = mediumReadWrite->vtbl->GetId(mediumReadWrite, &mediumReadWriteIdUtf16); + reportInternalErrorIfNS_FAILED("cannot get medium id"); + rc = mediumReadWrite->vtbl->GetFormat(mediumReadWrite, &mediumReadWriteFormatUtf16); + reportInternalErrorIfNS_FAILED("cannot get medium format"); + VBOX_UTF16_TO_UTF8(mediumReadWriteIdUtf16, &mediumReadWriteId); + VBOX_UTF16_TO_UTF8(mediumReadWriteFormatUtf16, &mediumReadWriteFormat); + char *uuidToReplace = NULL; + virSearchUuid(newMachineStorageControllersString, it+1, &uuidToReplace); + if (STRNEQ(uuidToReplace, mediumReadWriteId)) { + char *tmp = NULL; + tmp = virStrReplace(newMachineStorageControllersString, uuidToReplace, mediumReadWriteId); + VIR_FREE(newMachineStorageControllersString); + if (VIR_STRDUP(newMachineStorageControllersString, tmp) < 0) { + VIR_DEBUG("Error while using strdup"); + } + VIR_FREE(tmp); + } + buf = xmlBufferCreate(); + xmlNodeDump(buf, mediaRegistryNodeOriginal->doc, mediaRegistryNodeOriginal, 0, 0); + ignore_value(VIR_STRDUP(mediaRegistryNodeOriginalString, (char *)xmlBufferContent(buf))); + xmlBufferFree(buf); + if (mediaRegistryNodeOriginalString && (strstr(mediaRegistryNodeOriginalString, mediumReadWriteId) == NULL)) { + IMedium *mediumReadWriteParent = NULL; + rc = mediumReadWrite->vtbl->GetParent(mediumReadWrite, &mediumReadWriteParent); + reportInternalErrorIfNS_FAILED("cannot get disk parent"); + if (mediumReadWriteParent) { + PRUnichar *parentIdUtf16 = NULL; + char *parentId = NULL; + char *parentIdWithBrackets = NULL; + char *childIdWithBrackets = NULL; + rc = mediumReadWriteParent->vtbl->GetId(mediumReadWriteParent, &parentIdUtf16); + reportInternalErrorIfNS_FAILED("cannot get disk id"); + if (parentIdUtf16) { + VBOX_UTF16_TO_UTF8(parentIdUtf16, &parentId); + if (virAsprintf(&parentIdWithBrackets, "{%s}", parentId) != -1 && virAsprintf(&childIdWithBrackets, "{%s}", mediumReadWriteId) != -1) { + vboxSnapshotXmlAppendDiskToMediaRegistry(&(mediaRegistryNodeOriginal->children), parentIdWithBrackets, childIdWithBrackets, def->disks[it].file, mediumReadWriteFormat); + VIR_FREE(parentIdWithBrackets); + VIR_FREE(childIdWithBrackets); + } + VBOX_UTF8_FREE(parentId); + } + VBOX_RELEASE(mediumReadWriteParent); + VBOX_UTF16_FREE(parentIdUtf16); + } + } + VBOX_UTF8_FREE(mediumReadWriteFormat); + VBOX_UTF8_FREE(mediumReadWriteId); + VBOX_RELEASE(mediumReadWrite); + } + VBOX_UTF16_FREE(mediumReadWriteIdUtf16); + VBOX_UTF16_FREE(mediumReadWriteFormatUtf16); + VBOX_UTF16_FREE(locationReadWriteDiskUtf16); + } + vboxArrayRelease(&mediumAttachments); + + /*Generation of the skeleton*/ + + VBOX_UTF16_TO_UTF8((PRUnichar *)domiid.value, &domainUuid); + rc = machine->vtbl->GetName(machine, &machineNameUtf16); + reportInternalErrorIfNS_FAILED("cannot get machine name"); + rc = machine->vtbl->GetSnapshotFolder(machine, &snapshotFolderUtf16); + reportInternalErrorIfNS_FAILED("cannot get snapshot folder"); + char *snapshotFolder = NULL; + VBOX_UTF16_TO_UTF8(snapshotFolderUtf16, &snapshotFolder); + VBOX_UTF16_FREE(snapshotFolderUtf16); + VBOX_UTF16_TO_UTF8(machineNameUtf16, &machineName); + VBOX_UTF16_FREE(machineNameUtf16); + if (snapshotUuid == NULL) { + VIR_DEBUG("Bad initialization"); + } + if (virUUIDGenerate(snapshotUuid) == -1) { + VIR_DEBUG("Could not generate a snapshot uuid"); + } else { + virUUIDFormat(snapshotUuid, snapshotuuidstr); + VIR_DEBUG("UUID: %s", snapshotuuidstr); + } + rc = machine->vtbl->GetCurrentSnapshot(machine, &savedCurrentSnapshot); + reportInternalErrorIfNS_FAILED("cannot get current snapshot"); + if (savedCurrentSnapshot) { + savedCurrentSnapshot->vtbl->GetId(savedCurrentSnapshot, &savedCurrentSnapshotIid.value); + VBOX_UTF16_TO_UTF8(savedCurrentSnapshotIid.value, &savedCurrentSnapshotUuid); + vboxIIDUnalloc(&savedCurrentSnapshotIid); + VBOX_RELEASE(savedCurrentSnapshot); + } + rc = machine ->vtbl->GetLastStateChange(machine, &lastStateChange); + reportInternalErrorIfNS_FAILED("cannot get last state change"); + xmlBufferPtr skeletonBuf = vboxSnapshotGenerateXmlSkeleton(domainUuid, machineName, snapshotuuidstr, snapshotFolder, lastStateChange, isCurrent, savedCurrentSnapshotUuid); + /* Now we can reopen our file in tree mode and append node we want. */ + xmlDocPtr newXml = virXMLParse(NULL, (char *)skeletonBuf->content, NULL); + if (newXml) { + xmlNodePtr newXmlRootElement = xmlDocGetRootElement(newXml); + xmlNodePtr newXmlMachineNode = NULL; + xmlNodePtr snapshotNodeToAppend = NULL; + IMedium *array[] = { NULL }; + IProgress *progress = NULL; + vboxArray media = VBOX_ARRAY_INITIALIZER; + + vboxSnapshotXmlRetrieveMachineNode(newXmlRootElement, &newXmlMachineNode); + if (newXmlMachineNode) { + if (mediaRegistryNodeOriginal) { + vboxSnapshotXmlAddChild(newXmlMachineNode, mediaRegistryNodeOriginal); + } + if (extraDataNode) { + vboxSnapshotXmlAddChild(newXmlMachineNode, extraDataNode); + } + } + if (!snapshotHasParents) { + vboxSnapshotGenerateVboxXML(xmlDocGetRootElement(xml), newSnapshotStorageControllersString, NULL, def->name, virTimeStringThen(def->creationTime*1000), snapshotuuidstr, &snapshotNodeToAppend); + } else { + vboxSnapshotGenerateVboxXML(xmlDocGetRootElement(xml), newSnapshotStorageControllersString, def->parent, def->name, virTimeStringThen(def->creationTime*1000), snapshotuuidstr, &snapshotNodeToAppend); + } + if (snapshotNodeToAppend) { + if (xmlAddChild(newXmlMachineNode, xmlCopyNode(snapshotNodeToAppend, 1)) == NULL) { + VIR_DEBUG("Error while building the xml"); + } + } + if (newXmlMachineNode) { + if (machineHardwareNode) { + vboxSnapshotXmlAddChild(newXmlMachineNode, machineHardwareNode); + } + xmlParseInNodeContext(newXmlMachineNode, newMachineStorageControllersString, (int)strlen(newMachineStorageControllersString), 0, &modifiedMachineStorageControllersNode); + if (modifiedMachineStorageControllersNode) { + if (xmlAddChild(newXmlMachineNode, xmlCopyNode(modifiedMachineStorageControllersNode, 1)) == NULL) { + VIR_DEBUG("Could not add modifiedSnapshotStorageControllersNode node to snapshot node"); + } + } + } + xmlNewProp(newXmlRootElement, BAD_CAST "xmlns", BAD_CAST "http://www.innotek.de/VirtualBox-settings"); + rc = vboxArrayGetWithUintArg(&media, machine, machine->vtbl->Unregister, CleanupMode_DetachAllReturnHardDisksOnly); + for (it = 0; it < media.count; ++it) { + IMedium *disk = media.items[it]; + if (disk) + vboxDetachAndCloseDisks(dom, disk); + } +# if VBOX_API_VERSION == 4002 + rc = machine->vtbl->Delete(machine, 0, array, &progress); +# elif VBOX_API_VERSION >= 4003 + rc = machine->vtbl->DeleteConfig(machine, 0, array, &progress); +# endif + reportInternalErrorIfNS_FAILED("cannot delete machine"); + if (progress != NULL) { + progress->vtbl->WaitForCompletion(progress, -1); + VBOX_RELEASE(progress); + } + vboxArrayRelease(&media); + if (virFileMakePath(mdir_name(settingsFilePath_Utf8)) < 0) + virReportError(VIR_ERR_INTERNAL_ERROR, "%s" + " %s", + _("Cannot create path :"), + settingsFilePath_Utf8); + if (xmlSaveFormatFileEnc(settingsFilePath_Utf8, newXml, "ISO-8859-1", 1) <= 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s" + " %s", + _("Cannot save xml file to:"), + settingsFilePath_Utf8); + } + rc = data->vboxObj->vtbl->OpenMachine(data->vboxObj, settingsFilePath, &newMachine); + unsigned int failedCounter = 10; + while (NS_FAILED(rc) && (rc == 0x80004005 /*NSCOM standard error*/) && failedCounter != 0) { + /*There is some random fails happening and they are not the error + *supposed to happen. + *I didn't have some solution on #vbox-dev + *but it appears that with another try it works so that's why we are trying 10 times maximum + */ + rc = data->vboxObj->vtbl->OpenMachine(data->vboxObj, settingsFilePath, &newMachine); + failedCounter--; + } + reportInternalErrorIfNS_FAILED("cannot open machine"); + if (newMachine) { + rc = data->vboxObj->vtbl->RegisterMachine(data->vboxObj, newMachine); + reportInternalErrorIfNS_FAILED("cannot register machine"); + } + else { + ret = -1; + goto cleanup; + } + } + } + ret = 0; +cleanup: + xmlFreeDoc(xml); + return ret; +} +#endif static virDomainSnapshotPtr vboxDomainSnapshotCreateXML(virDomainPtr dom, const char *xmlDesc, @@ -5967,20 +6797,20 @@ vboxDomainSnapshotCreateXML(virDomainPtr dom, #else PRInt32 result; #endif +#if VBOX_API_VERSION >= 4002 + bool isCurrent = false; +#endif /* VBox has no snapshot metadata, so this flag is trivial. */ - virCheckFlags(VIR_DOMAIN_SNAPSHOT_CREATE_NO_METADATA, NULL); + virCheckFlags(VIR_DOMAIN_SNAPSHOT_CREATE_NO_METADATA | + VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE | + VIR_DOMAIN_SNAPSHOT_CREATE_CURRENT, NULL); if (!(def = virDomainSnapshotDefParseString(xmlDesc, data->caps, data->xmlopt, -1, VIR_DOMAIN_SNAPSHOT_PARSE_DISKS | VIR_DOMAIN_SNAPSHOT_PARSE_REDEFINE))) goto cleanup; - if (def->ndisks) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("disk snapshots not supported yet")); - goto cleanup; - } vboxIIDFromUUID(&domiid, dom->uuid); rc = VBOX_OBJECT_GET_MACHINE(domiid.value, &machine); if (NS_FAILED(rc)) { @@ -5988,6 +6818,14 @@ vboxDomainSnapshotCreateXML(virDomainPtr dom, _("no domain with matching UUID")); goto cleanup; } +#if VBOX_API_VERSION >= 4002 + isCurrent = flags & VIR_DOMAIN_SNAPSHOT_CREATE_CURRENT; + if (flags & VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE) { + vboxSnapshotRedefine(dom, def, isCurrent); + ret = virGetDomainSnapshot(dom, def->name); + goto cleanup; + } +#endif rc = machine->vtbl->GetState(machine, &state); if (NS_FAILED(rc)) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", @@ -7239,8 +8077,6 @@ cleanup: return ret; } - - static int vboxDomainSnapshotDelete(virDomainSnapshotPtr snapshot, unsigned int flags) -- 1.7.10.4
On 11/25/2013 07:23 PM, Manuel VIVES wrote:
The snapshots are saved in xml files, and then can be redefined --- src/vbox/vbox_tmpl.c | 852 +++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 844 insertions(+), 8 deletions(-)
diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c index e05ed97..23f8aab 100644 --- a/src/vbox/vbox_tmpl.c +++ b/src/vbox/vbox_tmpl.c @@ -61,6 +61,7 @@ #include "virstring.h" #include "virtime.h" #include "virutil.h" +#include "dirname.h"
/* This one changes from version to version. */ #if VBOX_API_VERSION == 2002 @@ -276,6 +277,12 @@ static vboxGlobalData *g_pVBoxGlobalData = NULL;
#endif /* VBOX_API_VERSION >= 4000 */
+/*This error is a bit specific + *In the VBOX API it is named E_ACCESSDENIED + *It is returned when the called object is not ready. In + *particular when we do any call on a disk which has been closed +*/ +#define VBOX_E_ACCESSDENIED 0x80070005
Is this code not defined anywhere in vbox API include files?
#define reportInternalErrorIfNS_FAILED(message) \ if (NS_FAILED(rc)) { \ virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _(message)); \ @@ -286,6 +293,8 @@ static vboxGlobalData *g_pVBoxGlobalData = NULL; static virDomainPtr vboxDomainDefineXML(virConnectPtr conn, const char *xml); static int vboxDomainCreate(virDomainPtr dom); static int vboxDomainUndefineFlags(virDomainPtr dom, unsigned int flags); +static virStorageVolPtr vboxStorageVolLookupByPath(virConnectPtr conn, const char *path); +static int vboxStorageDeleteOrClose(virStorageVolPtr vol, unsigned int flags, unsigned int flagDeleteOrClose);
The above line looks to be > 80 columns.
static void vboxDriverLock(vboxGlobalData *data) { virMutexLock(&data->lock); } @@ -5946,6 +5955,827 @@ cleanup: return snapshot; }
+#if VBOX_API_VERSION >=4002 +static void +vboxSnapshotXmlRetrieveSnapshotNodeByName(xmlNodePtr a_node, + const char *name, + xmlNodePtr *snap_node) +{ + xmlNodePtr cur_node = NULL; + + for (cur_node = a_node; cur_node; cur_node = cur_node->next) { + if (cur_node->type == XML_ELEMENT_NODE) { + if (!xmlStrcmp(cur_node->name, (const xmlChar *) "Snapshot") && + STREQ(virXMLPropString(cur_node, "name"), name)) {
Two problems here: 1) virXMLPropString() could return NULL in case it doesn't find the property you're looking for, and you haven't allowed for that. 2) virXMLPropString returns a *copy* of the string, and you need to VIR_FREE() it after you're finished with it. But you haven't even saved a copy of the pointer, so you have no way of doing that. So every time you call virXMLPropString(), you're either going to leak a string, or segv libvirtd.
+ *snap_node = cur_node; + return; + } + } + if (cur_node->children) + vboxSnapshotXmlRetrieveSnapshotNodeByName(cur_node->children, name, snap_node); + } +} + + + + +static int +vboxDetachAndCloseDisks(virDomainPtr dom, + IMedium *disk) +{ + VBOX_OBJECT_CHECK(dom->conn, int, -1); + nsresult rc; + PRUnichar *location = NULL; + vboxArray childrenDiskArray = VBOX_ARRAY_INITIALIZER; + virStorageVolPtr volPtr = NULL; + char *location_utf8 = NULL; + PRUint32 dummyState = 0; + size_t i = 0; + if (disk == NULL) { + VIR_DEBUG("Null pointer to disk"); + return -1;
If something is an error, and will end up failing an operation, then you need to log an error message rather than just outputting a debug message. This is true even if the error can only be caused by an internal problem in libvirt's own code.
+ } + rc = disk->vtbl->GetLocation(disk, &location); + if (rc == VBOX_E_ACCESSDENIED) { + VIR_DEBUG("Disk already closed");
Not sure if this is an actual error, or a valid occurrence (I'm guessing the former, since you skip the rest of the function on failure). If it shouldn't happen, then you should be logging an error.
+ goto cleanup; + } + reportInternalErrorIfNS_FAILED("cannot get disk location"); + rc = vboxArrayGet(&childrenDiskArray, disk, disk->vtbl->GetChildren); + reportInternalErrorIfNS_FAILED("cannot get children disks"); + for (i = 0; i < childrenDiskArray.count; ++i) { + IMedium *childDisk = childrenDiskArray.items[i]; + if (childDisk) { + vboxDetachAndCloseDisks(dom, childDisk); + } + } + rc = disk->vtbl->RefreshState(disk, &dummyState); + reportInternalErrorIfNS_FAILED("cannot refresh state"); + VBOX_UTF16_TO_UTF8(location, &location_utf8); + volPtr = vboxStorageVolLookupByPath(dom->conn, location_utf8); + + if (volPtr) { + VIR_DEBUG("Closing %s", location_utf8); + if (vboxStorageDeleteOrClose(volPtr, 0, VBOX_STORAGE_CLOSE_FLAG) != 0) { + VIR_DEBUG("Error while closing disk");
Same problem here.
+ } + } + VBOX_UTF8_FREE(location_utf8); +cleanup: + VBOX_UTF16_FREE(location); + vboxArrayRelease(&childrenDiskArray); + return ret; +} + +static void +vboxSnapshotXmlAddChild(xmlNodePtr parent, + xmlNodePtr child) +{ + /*Used in order to add child without writing the stuff concerning xml namespaces*/ + xmlBufferPtr tmpBuf = xmlBufferCreate(); + char *tmpString = NULL; + xmlNodePtr tmpNode = NULL; + xmlNodeDump(tmpBuf, parent->doc, child, 0, 0); + ignore_value(VIR_STRDUP(tmpString, (char *)xmlBufferContent(tmpBuf)));
I don't think it's safe to ignore the return of VIR_STRDUP here.
+ xmlParseInNodeContext(parent, tmpString, (int)strlen(tmpString), 0, &tmpNode); + if (tmpNode) { + if (xmlAddChild(parent, xmlCopyNode(tmpNode, 1)) == NULL) { + VIR_DEBUG("Error while adding %s to %s", (char *)tmpNode->name, (char *)parent->name); + } + } + xmlFree(tmpNode); + xmlBufferFree(tmpBuf); +} + +static void +vboxSnapshotXmlRetrieveMachineNode(xmlNodePtr root, + xmlNodePtr *machineNode) +{ + xmlNodePtr cur = root->xmlChildrenNode; + while (cur && xmlIsBlankNode(cur)) { + cur = cur -> next; + } + if (xmlStrcmp(cur->name, (const xmlChar *) "Machine")) { + VIR_DEBUG("Problem, found %s, Machine expected", (char *)cur->name); + return; + } + *machineNode = cur; +} + +static void +vboxSnapshotXmlRetrieveSnapshotsNode(xmlNodePtr snapshotNode, + xmlNodePtr *snapshotsNode) +{ + xmlNodePtr cur_node = NULL; + + for (cur_node = snapshotNode; cur_node; cur_node = cur_node->next) { + if (cur_node->type == XML_ELEMENT_NODE) { + if (!xmlStrcmp(cur_node->name, (const xmlChar *) "Snapshots")) { + *snapshotsNode = cur_node; + return; + } + } + if (cur_node->children) + vboxSnapshotXmlRetrieveSnapshotsNode(cur_node->children, snapshotsNode); + } +} + +static void +vboxSnapshotXmlRetrieveRootNodeByName(xmlNodePtr root, + const char *name, + xmlNodePtr *returnNode) +{ + xmlNodePtr cur = root->xmlChildrenNode; + while (cur && xmlIsBlankNode(cur)) { + cur = cur -> next; + } + if (xmlStrcmp(cur->name, (const xmlChar *) "Machine")) { + VIR_DEBUG("Problem, found %s, Machine expected", (char *)cur->name); + } + cur=cur->xmlChildrenNode; + while (cur != NULL) { + if (!xmlStrcmp(cur->name, (const xmlChar*) name)) { + *returnNode = cur; + return; + } + cur = cur -> next; + } +} + +static void +vboxSnapshotXmlAppendDiskToMediaRegistry(xmlNodePtr *inMediaRegistry, + char *parentId, + char *childId, + char *childLocation, + char *childFormat) +{ + /*This function will modify the inMediaregistry node to append all the informations about the child disk + */ + xmlNodePtr cur_node = NULL; + for (cur_node = *inMediaRegistry; cur_node; cur_node = cur_node->next) { + if (cur_node) { + if (cur_node->type == XML_ELEMENT_NODE + && !xmlStrcmp(cur_node->name, (const xmlChar *) "HardDisk") + && xmlHasProp(cur_node, BAD_CAST "uuid") != NULL + && STREQ((char *)xmlHasProp(cur_node, BAD_CAST "uuid")->children->content, parentId)) { + + xmlNodePtr childDiskNode = xmlNewTextChild(cur_node, NULL, BAD_CAST "HardDisk", NULL); + xmlNewProp(childDiskNode, BAD_CAST "uuid", BAD_CAST childId); + xmlNewProp(childDiskNode, BAD_CAST "location", BAD_CAST childLocation); + xmlNewProp(childDiskNode, BAD_CAST "format", BAD_CAST childFormat);
xmlNewProp can return an error on failure. But of course this function only returns void, so there's no way to report it to the caller anyway :-(
+ return; + } + } + if (&(cur_node->children)) + vboxSnapshotXmlAppendDiskToMediaRegistry(&(cur_node->children), parentId, childId, childLocation, childFormat); + + } +} + +static int +vboxSnapshotGenerateVboxXML(xmlNodePtr rootElementVboxXML, + char *storageControllerString, + char *parent, + char *defName, + char *timeStamp, + char *uuid, + xmlNodePtr *snapshotNodeReturned) +{ + xmlDocPtr doc; + xmlNodePtr snapshotNode; + xmlNodePtr snapshotsNode; + xmlNodePtr machineHardwareNode = NULL; + char *uuidstrWithBrackets = NULL; + char *timeVboxFormat = NULL; + int ret = -1; + /*We change the date format from "yyyy-MM-dd hh:mm:ss.msec+timeZone" to "yyyy-MM-ddThh:mm:ssZ"*/ + char *date = virStringSplit(virStringJoin((const char**)virStringSplit(virStringSplit(timeStamp, "+", 0)[0], " ", 0), "T"), ".", 0)[0];
This is compact, but doesn't protect against OOM errors (I know some people argue that once there is an OOM error, you're dead anyway so there isn't any point, but that isn't the philosophy that libvirt tries to follow). This is a statement about the vbox driver in general, btw, not just this particular occurrence in this patch...
+ + /*Retrieve hardware*/ + vboxSnapshotXmlRetrieveRootNodeByName(rootElementVboxXML, "Hardware", &machineHardwareNode); + /* Create a new XML DOM tree, to which the XML document will be + * written */ + doc = xmlNewDoc(BAD_CAST XML_DEFAULT_VERSION); + if (doc == NULL) { + VIR_DEBUG("Error creating the xml document tree\n"); + ret = -1; + goto cleanup; + } + /* Create a new XML node, to which the XML document will be + * appended */ + snapshotNode = xmlNewDocNode(doc, NULL, BAD_CAST "Snapshot", NULL); + if (snapshotNode == NULL) { + VIR_DEBUG("Error creating the xml node Snapshot\n"); + ret = -1; + goto cleanup; + } + if (virAsprintf(&uuidstrWithBrackets, "{%s}", uuid) != -1) { + xmlNewProp(snapshotNode, BAD_CAST "uuid", BAD_CAST uuidstrWithBrackets); + VIR_FREE(uuidstrWithBrackets); + } + xmlNewProp(snapshotNode, BAD_CAST "name", BAD_CAST defName); + if (virAsprintf(&timeVboxFormat, "%sZ", date) != -1) { + xmlNewProp(snapshotNode, BAD_CAST "timeStamp", BAD_CAST timeVboxFormat); + VIR_FREE(timeVboxFormat); + } + xmlDocSetRootElement(doc, snapshotNode); + if (machineHardwareNode) { + vboxSnapshotXmlAddChild(snapshotNode, machineHardwareNode); + } + xmlNodePtr listStorageControllerNodes; + xmlParseInNodeContext(snapshotNode, storageControllerString, (int)strlen(storageControllerString), 0, &listStorageControllerNodes); + if (listStorageControllerNodes) { + if (xmlAddChild(snapshotNode, xmlCopyNode(listStorageControllerNodes, 1)) == NULL) { + VIR_DEBUG("Could not add listStorageControllerNodes node to snapshot node"); + ret = -1; + goto cleanup; + } + } + snapshotsNode = xmlNewDocNode(doc, NULL, BAD_CAST "Snapshots", NULL); + if (snapshotsNode == NULL) { + VIR_DEBUG("Error creating the xml node Snapshots\n"); + ret = -1; + goto cleanup; + } + if (snapshotsNode) { + if (xmlAddChild(snapshotNode, xmlCopyNode(snapshotsNode, 1)) == NULL) { + VIR_DEBUG("Could not add snapshotsNode node to snapshot node"); + ret = -1; + goto cleanup; + } + } + if (parent) { + xmlNodePtr rootSnapshotNode = NULL; + xmlNodePtr parentNode = NULL; + xmlNodePtr parentSnapshotsNode = NULL; + vboxSnapshotXmlRetrieveRootNodeByName(rootElementVboxXML, "Snapshot", &rootSnapshotNode); + if (!rootSnapshotNode) { + VIR_DEBUG("Could not retrieve Snapshot node"); + ret = -1; + goto cleanup; + } + vboxSnapshotXmlRetrieveSnapshotNodeByName(rootSnapshotNode, parent, &parentNode); + if (!parentNode) { + VIR_DEBUG("Could not retrieve Snapshot node of %s", parent); + ret = -1; + goto cleanup; + } + vboxSnapshotXmlRetrieveSnapshotsNode(parentNode->children, &parentSnapshotsNode); + if (!parentSnapshotsNode) { + VIR_DEBUG("Could not retrieve Snapshots node"); + ret = -1; + goto cleanup; + } + if (xmlAddChild(parentSnapshotsNode, xmlCopyNode(snapshotNode, 1)) == NULL) { + VIR_DEBUG("Could not add snapshotsNode node to its parent"); + ret = -1; + goto cleanup; + } else { + *snapshotNodeReturned = xmlCopyNode(rootSnapshotNode, 1); + } + } else { + *snapshotNodeReturned = xmlCopyNode(snapshotNode, 1); + } + ret = 0; +cleanup: + return ret; +} + +static xmlBufferPtr +vboxSnapshotGenerateXmlSkeleton(char *domainUuid, + char *machineName, + char *snapshotUuid, + char *snapshotFolder, + PRInt64 lastStateChange, + bool isCurrent, + char *savedCurrentSnapshotUuid) +{ + char *snapshotUuidWithBrackets = NULL; + char *lastStateChangeStr = NULL; + /*Let's write a new xml + */ + xmlTextWriterPtr writer; + xmlBufferPtr buf = xmlBufferCreate(); + xmlBufferPtr ret = NULL; + int resultXml; + char *uuidWithBracket = NULL; + + /* Create a new XmlWriter with no compression. */ + writer = xmlNewTextWriterMemory(buf, 0); + if (writer == NULL) { + VIR_DEBUG("Error creating the xml writer\n"); + goto cleanup; + } + + /* Start the document with the xml default for the version, + * encoding ISO 8859-1 and the default for the standalone + * declaration. */ + resultXml = xmlTextWriterStartDocument(writer, NULL, "ISO-8859-1", NULL); + if (resultXml < 0) { + VIR_DEBUG("Error at xmlTextWriterStartDocument\n"); + goto cleanup; + } + /* Write a comment as child of root. + * Please observe, that the input to the xmlTextWriter functions + * HAS to be in UTF-8, even if the output XML is encoded + * in iso-8859-1 */ + resultXml = xmlTextWriterWriteFormatComment(writer, "WARNING: THIS IS AN AUTO-GENERATED FILE. CHANGES TO IT ARE LIKELY TO BE \n" + "OVERWRITTEN AND LOST.\n" + "Changes to this xml configuration should be made using Virtualbox \n" + "or other application using the libvirt API"); + if (resultXml < 0) { + VIR_DEBUG("Error at xmlTextWriterWriteComment\n"); + goto cleanup; + } + xmlTextWriterWriteRaw(writer, BAD_CAST "\n"); /*Break line after comment*/ + + /* Start an element named "VirtualBox". Since thist is the first + * element, this will be the root element of the document. */ + resultXml = xmlTextWriterStartElement(writer, BAD_CAST "VirtualBox"); + if (resultXml < 0) { + VIR_DEBUG("Error creating the xml node\n"); + goto cleanup; + } + resultXml = xmlTextWriterWriteAttribute(writer, BAD_CAST "version", BAD_CAST "1.12-linux"); + if (resultXml < 0) { + VIR_DEBUG("Error at xmlTextWriterWriteAttribute\n"); + goto cleanup; + } + /* Start an element named "Machine" as child of VirtualBox. */ + resultXml = xmlTextWriterStartElement(writer, BAD_CAST "Machine"); + if (resultXml < 0) { + VIR_DEBUG("Error at xmlTextWriterStartElement\n"); + goto cleanup; + } + /* Add an attribute with name "uuid" and value "{uuid}" to Machine. */ + if (virAsprintf(&uuidWithBracket, "{%s}", domainUuid) != -1) { + resultXml = xmlTextWriterWriteAttribute(writer, BAD_CAST "uuid", BAD_CAST uuidWithBracket); + if (resultXml < 0) { + VIR_DEBUG("Error at xmlTextWriterWriteAttribute\n"); + goto cleanup; + } + VIR_FREE(uuidWithBracket); + } + + if (!machineName) { + VIR_DEBUG("No machine name"); + goto cleanup; + } + resultXml = xmlTextWriterWriteAttribute(writer, BAD_CAST "name", BAD_CAST machineName); + if (resultXml < 0) { + VIR_DEBUG("Error at xmlTextWriterWriteAttribute\n"); + goto cleanup; + } + resultXml = xmlTextWriterWriteAttribute(writer, BAD_CAST "OSType", BAD_CAST "Other"); + if (resultXml < 0) { + VIR_DEBUG("Error at xmlTextWriterWriteAttribute\n"); + goto cleanup; + } + + if (!snapshotFolder) { + VIR_DEBUG("No machine snapshotFolder"); + goto cleanup; + } + resultXml = xmlTextWriterWriteAttribute(writer, BAD_CAST "snapshotFolder", BAD_CAST snapshotFolder); + if (resultXml < 0) { + VIR_DEBUG("Error at xmlTextWriterWriteAttribute\n"); + goto cleanup; + } + if (virAsprintf(&lastStateChangeStr, "%ld", lastStateChange) < 0) { + VIR_DEBUG("Could not convert lastStateChange from long to string"); + goto cleanup; + } + resultXml = xmlTextWriterWriteAttribute(writer, BAD_CAST "lastStateChange", BAD_CAST lastStateChangeStr); + if (resultXml < 0) { + VIR_DEBUG("Error at xmlTextWriterWriteAttribute\n"); + goto cleanup; + } + /* Current Snapshot + * https://www.virtualbox.org/sdkref/interface_i_machine.html#ac785dbe04eccc079... + * There is always a current snapshot, except when there is no snapshot (obvious) or when they have all been deleted (obvious) + * or when the current snapshot is deleted and it does not have a parent (not so obvious). + * This can happen only when the last snapshot is deleted because there can only be one snapshot with no parent, the first one. + * For the moment we always put the snapshot we are defining as current snapshot. When all the snapshots are redefined, you can always set + * the current snapshot to one you have previously saved. + */
Reformat to fit in 80 columns. I'm going to have to stop here, as there is just too much vbox-centric code that I don't understand, and already a lot of re-occurring problems I've pointed out that should be fixed everywhere in all the patches before a rebase/resubmit. If there is someone else who understands the vbox driver (and vbox itself) better than me, I would encourage them to jump in when revised patches are posted to do a lower level review - I can still look for problems related to the way that libvirt internal APIs (and do some small extent libxml APIs) are called, but can't even *run* the code to test for correctness.
On 11/25/2013 07:23 PM, Manuel VIVES wrote:
The snapshots are saved in xml files, and then can be redefined ---
src/vbox/vbox_tmpl.c | 852 +++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 844 insertions(+), 8 deletions(-)
diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c index e05ed97..23f8aab 100644 --- a/src/vbox/vbox_tmpl.c +++ b/src/vbox/vbox_tmpl.c @@ -61,6 +61,7 @@
#include "virstring.h" #include "virtime.h" #include "virutil.h"
+#include "dirname.h"
/* This one changes from version to version. */ #if VBOX_API_VERSION == 2002
@@ -276,6 +277,12 @@ static vboxGlobalData *g_pVBoxGlobalData = NULL;
#endif /* VBOX_API_VERSION >= 4000 */
+/*This error is a bit specific + *In the VBOX API it is named E_ACCESSDENIED + *It is returned when the called object is not ready. In + *particular when we do any call on a disk which has been closed +*/ +#define VBOX_E_ACCESSDENIED 0x80070005
Is this code not defined anywhere in vbox API include files?
I did not find this code anywhere in the API, so I defined it here.
#define reportInternalErrorIfNS_FAILED(message) \
if (NS_FAILED(rc)) { \
virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _(message)); \
@@ -286,6 +293,8 @@ static vboxGlobalData *g_pVBoxGlobalData = NULL;
static virDomainPtr vboxDomainDefineXML(virConnectPtr conn, const char *xml); static int vboxDomainCreate(virDomainPtr dom); static int vboxDomainUndefineFlags(virDomainPtr dom, unsigned int flags);
+static virStorageVolPtr vboxStorageVolLookupByPath(virConnectPtr conn, const char *path); +static int vboxStorageDeleteOrClose(virStorageVolPtr vol, unsigned int flags, unsigned int flagDeleteOrClose);
The above line looks to be > 80 columns.
static void vboxDriverLock(vboxGlobalData *data) {
virMutexLock(&data->lock);
}
@@ -5946,6 +5955,827 @@ cleanup: return snapshot;
}
+#if VBOX_API_VERSION >=4002 +static void +vboxSnapshotXmlRetrieveSnapshotNodeByName(xmlNodePtr a_node, + const char *name, + xmlNodePtr *snap_node) +{ + xmlNodePtr cur_node = NULL; + + for (cur_node = a_node; cur_node; cur_node = cur_node->next) { + if (cur_node->type == XML_ELEMENT_NODE) { + if (!xmlStrcmp(cur_node->name, (const xmlChar *) "Snapshot") && + STREQ(virXMLPropString(cur_node, "name"), name)) {
Two problems here:
1) virXMLPropString() could return NULL in case it doesn't find the property you're looking for, and you haven't allowed for that.
2) virXMLPropString returns a *copy* of the string, and you need to VIR_FREE() it after you're finished with it. But you haven't even saved a copy of the pointer, so you have no way of doing that.
So every time you call virXMLPropString(), you're either going to leak a string, or segv libvirtd.
+ *snap_node = cur_node; + return; + } + } + if (cur_node->children) + vboxSnapshotXmlRetrieveSnapshotNodeByName(cur_node->children, name, snap_node); + } +} + + + + +static int +vboxDetachAndCloseDisks(virDomainPtr dom, + IMedium *disk) +{ + VBOX_OBJECT_CHECK(dom->conn, int, -1); + nsresult rc; + PRUnichar *location = NULL; + vboxArray childrenDiskArray = VBOX_ARRAY_INITIALIZER; + virStorageVolPtr volPtr = NULL; + char *location_utf8 = NULL; + PRUint32 dummyState = 0; + size_t i = 0; + if (disk == NULL) { + VIR_DEBUG("Null pointer to disk"); + return -1;
If something is an error, and will end up failing an operation, then you need to log an error message rather than just outputting a debug message. This is true even if the error can only be caused by an internal problem in libvirt's own code.
+ } + rc = disk->vtbl->GetLocation(disk, &location); + if (rc == VBOX_E_ACCESSDENIED) { + VIR_DEBUG("Disk already closed");
Not sure if this is an actual error, or a valid occurrence (I'm guessing the former, since you skip the rest of the function on failure). If it shouldn't happen, then you should be logging an error.
This is a valid occurence, it just means that the disk has already been closed (so it is not useful to continue in the function).
+ goto cleanup; + } + reportInternalErrorIfNS_FAILED("cannot get disk location"); + rc = vboxArrayGet(&childrenDiskArray, disk, disk->vtbl->GetChildren); + reportInternalErrorIfNS_FAILED("cannot get children disks"); + for (i = 0; i < childrenDiskArray.count; ++i) { + IMedium *childDisk = childrenDiskArray.items[i]; + if (childDisk) { + vboxDetachAndCloseDisks(dom, childDisk); + } + } + rc = disk->vtbl->RefreshState(disk, &dummyState); + reportInternalErrorIfNS_FAILED("cannot refresh state"); + VBOX_UTF16_TO_UTF8(location, &location_utf8); + volPtr = vboxStorageVolLookupByPath(dom->conn, location_utf8); + + if (volPtr) { + VIR_DEBUG("Closing %s", location_utf8); + if (vboxStorageDeleteOrClose(volPtr, 0, VBOX_STORAGE_CLOSE_FLAG) != 0) { + VIR_DEBUG("Error while closing disk");
Same problem here.
+ } + } + VBOX_UTF8_FREE(location_utf8); +cleanup: + VBOX_UTF16_FREE(location); + vboxArrayRelease(&childrenDiskArray); + return ret; +} + +static void +vboxSnapshotXmlAddChild(xmlNodePtr parent, + xmlNodePtr child) +{ + /*Used in order to add child without writing the stuff concerning xml namespaces*/ + xmlBufferPtr tmpBuf = xmlBufferCreate(); + char *tmpString = NULL; + xmlNodePtr tmpNode = NULL; + xmlNodeDump(tmpBuf, parent->doc, child, 0, 0); + ignore_value(VIR_STRDUP(tmpString, (char *)xmlBufferContent(tmpBuf)));
I don't think it's safe to ignore the return of VIR_STRDUP here.
+ xmlParseInNodeContext(parent, tmpString, (int)strlen(tmpString), 0, &tmpNode); + if (tmpNode) { + if (xmlAddChild(parent, xmlCopyNode(tmpNode, 1)) == NULL) { + VIR_DEBUG("Error while adding %s to %s", (char *)tmpNode->name, (char *)parent->name); + } + } + xmlFree(tmpNode); + xmlBufferFree(tmpBuf); +} + +static void +vboxSnapshotXmlRetrieveMachineNode(xmlNodePtr root, + xmlNodePtr *machineNode) +{ + xmlNodePtr cur = root->xmlChildrenNode; + while (cur && xmlIsBlankNode(cur)) { + cur = cur -> next; + } + if (xmlStrcmp(cur->name, (const xmlChar *) "Machine")) { + VIR_DEBUG("Problem, found %s, Machine expected", (char *)cur->name); + return; + } + *machineNode = cur; +} + +static void +vboxSnapshotXmlRetrieveSnapshotsNode(xmlNodePtr snapshotNode, + xmlNodePtr *snapshotsNode) +{ + xmlNodePtr cur_node = NULL; + + for (cur_node = snapshotNode; cur_node; cur_node = cur_node->next) { + if (cur_node->type == XML_ELEMENT_NODE) { + if (!xmlStrcmp(cur_node->name, (const xmlChar *) "Snapshots")) { + *snapshotsNode = cur_node; + return; + } + } + if (cur_node->children) + vboxSnapshotXmlRetrieveSnapshotsNode(cur_node->children, snapshotsNode); + } +} + +static void +vboxSnapshotXmlRetrieveRootNodeByName(xmlNodePtr root, + const char *name, + xmlNodePtr *returnNode) +{ + xmlNodePtr cur = root->xmlChildrenNode; + while (cur && xmlIsBlankNode(cur)) { + cur = cur -> next; + } + if (xmlStrcmp(cur->name, (const xmlChar *) "Machine")) { + VIR_DEBUG("Problem, found %s, Machine expected", (char *)cur->name); + } + cur=cur->xmlChildrenNode; + while (cur != NULL) { + if (!xmlStrcmp(cur->name, (const xmlChar*) name)) { + *returnNode = cur; + return; + } + cur = cur -> next; + } +} + +static void +vboxSnapshotXmlAppendDiskToMediaRegistry(xmlNodePtr *inMediaRegistry, + char *parentId, + char *childId, + char *childLocation, + char *childFormat) +{ + /*This function will modify the inMediaregistry node to append all the informations about the child disk + */ + xmlNodePtr cur_node = NULL; + for (cur_node = *inMediaRegistry; cur_node; cur_node = cur_node->next) { + if (cur_node) { + if (cur_node->type == XML_ELEMENT_NODE + && !xmlStrcmp(cur_node->name, (const xmlChar *) "HardDisk") + && xmlHasProp(cur_node, BAD_CAST "uuid") != NULL + && STREQ((char *)xmlHasProp(cur_node, BAD_CAST "uuid")->children->content, parentId)) { + + xmlNodePtr childDiskNode = xmlNewTextChild(cur_node, NULL, BAD_CAST "HardDisk", NULL); + xmlNewProp(childDiskNode, BAD_CAST "uuid", BAD_CAST childId); + xmlNewProp(childDiskNode, BAD_CAST "location", BAD_CAST childLocation); + xmlNewProp(childDiskNode, BAD_CAST "format", BAD_CAST childFormat);
xmlNewProp can return an error on failure. But of course this function only returns void, so there's no way to report it to the caller anyway :-(
+ return; + } + } + if (&(cur_node->children)) + vboxSnapshotXmlAppendDiskToMediaRegistry(&(cur_node->children), parentId, childId, childLocation, childFormat); + + } +} + +static int +vboxSnapshotGenerateVboxXML(xmlNodePtr rootElementVboxXML, + char *storageControllerString, + char *parent, + char *defName, + char *timeStamp, + char *uuid, + xmlNodePtr *snapshotNodeReturned) +{ + xmlDocPtr doc; + xmlNodePtr snapshotNode; + xmlNodePtr snapshotsNode; + xmlNodePtr machineHardwareNode = NULL; + char *uuidstrWithBrackets = NULL; + char *timeVboxFormat = NULL; + int ret = -1; + /*We change the date format from "yyyy-MM-dd hh:mm:ss.msec+timeZone" to "yyyy-MM-ddThh:mm:ssZ"*/ + char *date = virStringSplit(virStringJoin((const char**)virStringSplit(virStringSplit(timeStamp, "+", 0)[0], " ", 0), "T"), ".", 0)[0];
This is compact, but doesn't protect against OOM errors (I know some people argue that once there is an OOM error, you're dead anyway so there isn't any point, but that isn't the philosophy that libvirt tries to follow). This is a statement about the vbox driver in general, btw, not just this particular occurrence in this patch...
+ + /*Retrieve hardware*/ + vboxSnapshotXmlRetrieveRootNodeByName(rootElementVboxXML, "Hardware", &machineHardwareNode); + /* Create a new XML DOM tree, to which the XML document will be + * written */ + doc = xmlNewDoc(BAD_CAST XML_DEFAULT_VERSION); + if (doc == NULL) { + VIR_DEBUG("Error creating the xml document tree\n"); + ret = -1; + goto cleanup; + } + /* Create a new XML node, to which the XML document will be + * appended */ + snapshotNode = xmlNewDocNode(doc, NULL, BAD_CAST "Snapshot", NULL); + if (snapshotNode == NULL) { + VIR_DEBUG("Error creating the xml node Snapshot\n"); + ret = -1; + goto cleanup; + } + if (virAsprintf(&uuidstrWithBrackets, "{%s}", uuid) != -1) { + xmlNewProp(snapshotNode, BAD_CAST "uuid", BAD_CAST uuidstrWithBrackets); + VIR_FREE(uuidstrWithBrackets); + } + xmlNewProp(snapshotNode, BAD_CAST "name", BAD_CAST defName); + if (virAsprintf(&timeVboxFormat, "%sZ", date) != -1) { + xmlNewProp(snapshotNode, BAD_CAST "timeStamp", BAD_CAST timeVboxFormat); + VIR_FREE(timeVboxFormat); + } + xmlDocSetRootElement(doc, snapshotNode); + if (machineHardwareNode) { + vboxSnapshotXmlAddChild(snapshotNode, machineHardwareNode); + } + xmlNodePtr listStorageControllerNodes; + xmlParseInNodeContext(snapshotNode, storageControllerString, (int)strlen(storageControllerString), 0, &listStorageControllerNodes); + if (listStorageControllerNodes) { + if (xmlAddChild(snapshotNode, xmlCopyNode(listStorageControllerNodes, 1)) == NULL) { + VIR_DEBUG("Could not add listStorageControllerNodes node to snapshot node"); + ret = -1; + goto cleanup; + } + } + snapshotsNode = xmlNewDocNode(doc, NULL, BAD_CAST "Snapshots", NULL); + if (snapshotsNode == NULL) { + VIR_DEBUG("Error creating the xml node Snapshots\n"); + ret = -1; + goto cleanup; + } + if (snapshotsNode) { + if (xmlAddChild(snapshotNode, xmlCopyNode(snapshotsNode, 1)) == NULL) { + VIR_DEBUG("Could not add snapshotsNode node to snapshot node"); + ret = -1; + goto cleanup; + } + } + if (parent) { + xmlNodePtr rootSnapshotNode = NULL; + xmlNodePtr parentNode = NULL; + xmlNodePtr parentSnapshotsNode = NULL; + vboxSnapshotXmlRetrieveRootNodeByName(rootElementVboxXML, "Snapshot", &rootSnapshotNode); + if (!rootSnapshotNode) { + VIR_DEBUG("Could not retrieve Snapshot node"); + ret = -1; + goto cleanup; + } + vboxSnapshotXmlRetrieveSnapshotNodeByName(rootSnapshotNode, parent, &parentNode); + if (!parentNode) { + VIR_DEBUG("Could not retrieve Snapshot node of %s", parent); + ret = -1; + goto cleanup; + } + vboxSnapshotXmlRetrieveSnapshotsNode(parentNode->children, &parentSnapshotsNode); + if (!parentSnapshotsNode) { + VIR_DEBUG("Could not retrieve Snapshots node"); + ret = -1; + goto cleanup; + } + if (xmlAddChild(parentSnapshotsNode, xmlCopyNode(snapshotNode, 1)) == NULL) { + VIR_DEBUG("Could not add snapshotsNode node to its parent"); + ret = -1; + goto cleanup; + } else { + *snapshotNodeReturned = xmlCopyNode(rootSnapshotNode, 1); + } + } else { + *snapshotNodeReturned = xmlCopyNode(snapshotNode, 1); + } + ret = 0; +cleanup: + return ret; +} + +static xmlBufferPtr +vboxSnapshotGenerateXmlSkeleton(char *domainUuid, + char *machineName, + char *snapshotUuid, + char *snapshotFolder, + PRInt64 lastStateChange, + bool isCurrent, + char *savedCurrentSnapshotUuid) +{ + char *snapshotUuidWithBrackets = NULL; + char *lastStateChangeStr = NULL; + /*Let's write a new xml + */ + xmlTextWriterPtr writer; + xmlBufferPtr buf = xmlBufferCreate(); + xmlBufferPtr ret = NULL; + int resultXml; + char *uuidWithBracket = NULL; + + /* Create a new XmlWriter with no compression. */ + writer = xmlNewTextWriterMemory(buf, 0); + if (writer == NULL) { + VIR_DEBUG("Error creating the xml writer\n"); + goto cleanup; + } + + /* Start the document with the xml default for the version, + * encoding ISO 8859-1 and the default for the standalone + * declaration. */ + resultXml = xmlTextWriterStartDocument(writer, NULL, "ISO-8859-1", NULL); + if (resultXml < 0) { + VIR_DEBUG("Error at xmlTextWriterStartDocument\n"); + goto cleanup; + } + /* Write a comment as child of root. + * Please observe, that the input to the xmlTextWriter functions + * HAS to be in UTF-8, even if the output XML is encoded + * in iso-8859-1 */ + resultXml = xmlTextWriterWriteFormatComment(writer, "WARNING: THIS IS AN AUTO-GENERATED FILE. CHANGES TO IT ARE LIKELY TO BE \n" + "OVERWRITTEN AND LOST.\n" + "Changes to this xml configuration should be made using Virtualbox \n" + "or other application using the libvirt API"); + if (resultXml < 0) { + VIR_DEBUG("Error at xmlTextWriterWriteComment\n"); + goto cleanup; + } + xmlTextWriterWriteRaw(writer, BAD_CAST "\n"); /*Break line after comment*/ + + /* Start an element named "VirtualBox". Since thist is the first + * element, this will be the root element of the document. */ + resultXml = xmlTextWriterStartElement(writer, BAD_CAST "VirtualBox"); + if (resultXml < 0) { + VIR_DEBUG("Error creating the xml node\n"); + goto cleanup; + } + resultXml = xmlTextWriterWriteAttribute(writer, BAD_CAST "version", BAD_CAST "1.12-linux"); + if (resultXml < 0) { + VIR_DEBUG("Error at xmlTextWriterWriteAttribute\n"); + goto cleanup; + } + /* Start an element named "Machine" as child of VirtualBox. */ + resultXml = xmlTextWriterStartElement(writer, BAD_CAST "Machine"); + if (resultXml < 0) { + VIR_DEBUG("Error at xmlTextWriterStartElement\n"); + goto cleanup; + } + /* Add an attribute with name "uuid" and value "{uuid}" to Machine. */ + if (virAsprintf(&uuidWithBracket, "{%s}", domainUuid) != -1) { + resultXml = xmlTextWriterWriteAttribute(writer, BAD_CAST "uuid", BAD_CAST uuidWithBracket); + if (resultXml < 0) { + VIR_DEBUG("Error at xmlTextWriterWriteAttribute\n"); + goto cleanup; + } + VIR_FREE(uuidWithBracket); + } + + if (!machineName) { + VIR_DEBUG("No machine name"); + goto cleanup; + } + resultXml = xmlTextWriterWriteAttribute(writer, BAD_CAST "name", BAD_CAST machineName); + if (resultXml < 0) { + VIR_DEBUG("Error at xmlTextWriterWriteAttribute\n"); + goto cleanup; + } + resultXml = xmlTextWriterWriteAttribute(writer, BAD_CAST "OSType", BAD_CAST "Other"); + if (resultXml < 0) { + VIR_DEBUG("Error at xmlTextWriterWriteAttribute\n"); + goto cleanup; + } + + if (!snapshotFolder) { + VIR_DEBUG("No machine snapshotFolder"); + goto cleanup; + } + resultXml = xmlTextWriterWriteAttribute(writer, BAD_CAST "snapshotFolder", BAD_CAST snapshotFolder); + if (resultXml < 0) { + VIR_DEBUG("Error at xmlTextWriterWriteAttribute\n"); + goto cleanup; + } + if (virAsprintf(&lastStateChangeStr, "%ld", lastStateChange) < 0) { + VIR_DEBUG("Could not convert lastStateChange from long to string"); + goto cleanup; + } + resultXml = xmlTextWriterWriteAttribute(writer, BAD_CAST "lastStateChange", BAD_CAST lastStateChangeStr); + if (resultXml < 0) { + VIR_DEBUG("Error at xmlTextWriterWriteAttribute\n"); + goto cleanup; + } + /* Current Snapshot + * https://www.virtualbox.org/sdkref/interface_i_machine.html#ac785dbe04ecc c0793d949d6940202767 + * There is always a current snapshot, except when there is no snapshot (obvious) or when they have all been deleted (obvious) + * or when the current snapshot is deleted and it does not have a parent (not so obvious). + * This can happen only when the last snapshot is deleted because there can only be one snapshot with no parent, the first one. + * For the moment we always put the snapshot we are defining as current snapshot. When all the snapshots are redefined, you can always set + * the current snapshot to one you have previously saved. + */
Reformat to fit in 80 columns.
I'm going to have to stop here, as there is just too much vbox-centric code that I don't understand, and already a lot of re-occurring problems I've pointed out that should be fixed everywhere in all the patches before a rebase/resubmit.
If there is someone else who understands the vbox driver (and vbox itself) better than me, I would encourage them to jump in when revised patches are posted to do a lower level review - I can still look for problems related to the way that libvirt internal APIs (and do some small extent libxml APIs) are called, but can't even *run* the code to test for correctness.
All the informations concerning snapshots (and snapshot disks) will be deleted from the vbox xml. But the differencing disks will be kept so you will be able to redefine the snapshots. --- src/vbox/vbox_tmpl.c | 391 ++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 391 insertions(+) diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c index 23f8aab..9b50c2e 100644 --- a/src/vbox/vbox_tmpl.c +++ b/src/vbox/vbox_tmpl.c @@ -6132,6 +6132,68 @@ vboxSnapshotXmlAppendDiskToMediaRegistry(xmlNodePtr *inMediaRegistry, } } + +static void +vboxRemoveAllDisksExceptParentFromMediaRegistry(xmlNodePtr mediaRegistryNode){ + xmlNodePtr cur_node = NULL; + for (cur_node = mediaRegistryNode; cur_node; cur_node = cur_node->next) { + if (cur_node) { + if (cur_node->type == XML_ELEMENT_NODE + && !xmlStrcmp(cur_node->name, (const xmlChar *) "HardDisk")) { + xmlNodePtr child = NULL; + for (child = cur_node->children; child; child = child->next) { + /*We look over all the children + *If there is a node element, we delete it + */ + if (child->type == XML_ELEMENT_NODE) { + xmlUnlinkNode(child); + xmlFreeNode(child); + } + } + } + } + if ((cur_node->children)) + vboxRemoveAllDisksExceptParentFromMediaRegistry((cur_node->children)); + } +} + +static void +vboxRemoveDiskFromMediaRegistryIfNoChildren(xmlNodePtr mediaRegistryNode, + char *diskLocation) +{ + /* + *This function will remove a disk from the media registry only if it doesn't + *have any children + */ + xmlNodePtr cur_node = NULL; + for (cur_node = mediaRegistryNode; cur_node; cur_node = cur_node->next) { + if (cur_node) { + if (cur_node->type == XML_ELEMENT_NODE + && !xmlStrcmp(cur_node->name, (const xmlChar *) "HardDisk") + && xmlHasProp(cur_node, BAD_CAST "location") != NULL + && strstr(diskLocation, (char *)xmlHasProp(cur_node, BAD_CAST "location")->children->content) != NULL) { + + xmlNodePtr child = NULL; + bool deleteNode = true; + for (child = cur_node->children; child; child = child->next) { + /*We look over all the children + *If there is a node element, we don't delete it + */ + if (child->type == XML_ELEMENT_NODE) + deleteNode = false; + } + if (deleteNode) { + xmlUnlinkNode(cur_node); + xmlFreeNode(cur_node); + } + return; + } + } + if ((cur_node->children)) + vboxRemoveDiskFromMediaRegistryIfNoChildren((cur_node->children), diskLocation); + } +} + static int vboxSnapshotGenerateVboxXML(xmlNodePtr rootElementVboxXML, char *storageControllerString, @@ -8076,8 +8138,319 @@ cleanup: vboxArrayRelease(&children); return ret; } +#if VBOX_API_VERSION >= 4002 +static int +vboxCloseDisk(virDomainPtr dom, + IMedium *baseDisk) { + VBOX_OBJECT_CHECK(dom->conn, int, -1); + nsresult rc; + vboxArray childrenDiskArray = VBOX_ARRAY_INITIALIZER; + size_t i = 0; + if (!baseDisk) + return -1; + + rc = vboxArrayGet(&childrenDiskArray, baseDisk, baseDisk->vtbl->GetChildren); + if (NS_FAILED(rc)) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("could not get children disks")); + ret = -1; + goto cleanup; + } + for (i=0; i < childrenDiskArray.count; ++i) + vboxCloseDisk(dom, childrenDiskArray.items[i]); + + baseDisk->vtbl->Close(baseDisk); + ret = 0; +cleanup: + vboxArrayRelease(&childrenDiskArray); + return ret; +} static int +vboxDomainSnapshotDeleteMetadataOnly(virDomainSnapshotPtr snapshot) +{ + /*This function will remove the node in the vbox xml corresponding to + *the snapshot. It is usually called by vboxDomainSnapshotDelete() with + *the flag VIR_DOMAIN_SNAPSHOT_DELETE_METADATA_ONLY. + *If you want to use it anywhere else, be careful, if the snapshot you want to delete has children, + *the result is not granted, they will probably will be deleted in the xml, but you may have + *a problem with hard drives + * + *If the snapshot which is being deleted is the current, we will set the current snapshot of the machine to + *its parent. + * + *Before the writing of the modified xml file, we undefine the machine from vbox + *After the modification, we redefine the machine + */ + + virDomainPtr dom = snapshot->domain; + VBOX_OBJECT_CHECK(dom->conn, int, -1); + IMachine *machine = NULL; + IMachine *newMachine = NULL; + nsresult rc; + ISnapshot *snap = NULL; + vboxIID domiid = VBOX_IID_INITIALIZER; + vboxArray machineDisks = VBOX_ARRAY_INITIALIZER; + IMedium *dummyArray[] = { NULL }; + PRUnichar *settingsFilePath = NULL; + char *settingsFilePathUtf8 = NULL; + xmlDocPtr xml = NULL; + xmlNodePtr xmlRootElement = NULL; + xmlNodePtr snapshotToDelete = NULL; + IProgress *progress = NULL; + vboxIID snapIid = VBOX_IID_INITIALIZER; + char *snapshotUuidStr = NULL; + char *snapshotUuidStrWithBrackets = NULL; + ISnapshot *parentSnapshot = NULL; + vboxIID parentIId = VBOX_IID_INITIALIZER; + char *parentSnapshotUuidStr = NULL; + char *parentSnapshotUuidStrWithBrackets = NULL; + virDomainSnapshotDefPtr def = NULL; + char *snapXmlDesc = NULL; + xmlNodePtr mediaRegistryNode = NULL; + xmlNodePtr storageControllersNode = NULL; + xmlBufferPtr buf = xmlBufferCreate(); + xmlNodePtr newStorageControllersNode = NULL; + xmlNodePtr machineNode = NULL; + bool snapshotHasParent = false; + char *snapshotStorageControllersString = NULL; + size_t i = 0; + unsigned int numberOfDisks = 0; + unsigned int failedCounter = 10; + + + VIR_DEBUG("vboxDomainSnapshotDeleteMetadataOnly()"); + vboxIIDFromUUID(&domiid, dom->uuid); + rc = VBOX_OBJECT_GET_MACHINE(domiid.value, &machine); + if (NS_FAILED(rc)) { + virReportError(VIR_ERR_NO_DOMAIN, "%s", + _("no domain with matching UUID")); + goto cleanup; + } + + /*Get snapshot*/ + snap = vboxDomainSnapshotGet(data, dom, machine, snapshot->name); + if (!snap) + goto cleanup; + rc = snap->vtbl->GetId(snap, &snapIid.value); + if (NS_FAILED(rc)) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("cannot get snapshot uuid")); + ret = -1; + goto cleanup; + } + snapXmlDesc = vboxDomainSnapshotGetXMLDesc(snapshot, 0); + if (!(def = virDomainSnapshotDefParseString(snapXmlDesc, data->caps, + data->xmlopt, -1, VIR_DOMAIN_SNAPSHOT_PARSE_DISKS | VIR_DOMAIN_SNAPSHOT_PARSE_REDEFINE))) + goto cleanup; + snapshotHasParent = !(def->parent == NULL); + /*Get its parent because it's possible that we have to put the parent uuid in + *the current snapshot attribute*/ + rc = snap->vtbl->GetParent(snap, &parentSnapshot); + reportInternalErrorIfNS_FAILED("cannot get parent snapshot"); + if (parentSnapshot) { + rc = parentSnapshot->vtbl->GetId(parentSnapshot, &parentIId.value); + reportInternalErrorIfNS_FAILED("cannot get parent snapshot id"); + VBOX_UTF16_TO_UTF8(parentIId.value, &parentSnapshotUuidStr); + if (virAsprintf(&parentSnapshotUuidStrWithBrackets, "{%s}", parentSnapshotUuidStr) < 0) { + VIR_DEBUG("problem while using virasprintf"); + goto cleanup; + } + } + + rc = machine->vtbl->GetSettingsFilePath(machine, &settingsFilePath); + reportInternalErrorIfNS_FAILED("cannot get settings file path"); + + /*We ask libxml2 to parse the xmlfile*/ + VBOX_UTF16_TO_UTF8(settingsFilePath, &settingsFilePathUtf8); + xml = virXMLParse(settingsFilePathUtf8, NULL, NULL); + xmlKeepBlanksDefault(1); + if (!xml) { + VIR_DEBUG("Cannot parse settings file"); + ret = -1; + goto cleanup; + } + + xmlRootElement = xmlDocGetRootElement(xml); + if (!xmlRootElement) { + VIR_DEBUG("Cannot find any root element in the xml"); + ret = -1; + goto cleanup; + } + + vboxSnapshotXmlRetrieveSnapshotNodeByName(xmlRootElement->children, snapshot->name, &snapshotToDelete); + if (!snapshotToDelete) { + VIR_DEBUG("Cannot find any snapshot to delete"); + ret = -1; + goto cleanup; + } + /*We manage the case where the snapshot we want to delete is the current one */ + VBOX_UTF16_TO_UTF8(snapIid.value, &snapshotUuidStr); + if (virAsprintf(&snapshotUuidStrWithBrackets, "{%s}", snapshotUuidStr) < 0) { + VIR_DEBUG("problem while using virasprintf"); + ret = -1; + goto cleanup; + } + vboxSnapshotXmlRetrieveMachineNode(xmlRootElement, &machineNode); + /*we change the current snapshot if necessary*/ + if (STREQ(virXMLPropString(machineNode, "currentSnapshot"), snapshotUuidStrWithBrackets)) { + if (parentSnapshotUuidStrWithBrackets != NULL) { + xmlSetProp(machineNode, BAD_CAST "currentSnapshot", BAD_CAST parentSnapshotUuidStrWithBrackets); + } else { + xmlUnsetProp(machineNode, BAD_CAST "currentSnapshot"); + } + } + vboxSnapshotXmlRetrieveRootNodeByName(xmlRootElement, "MediaRegistry", &mediaRegistryNode); + if (!mediaRegistryNode) { + VIR_DEBUG("Problem while retrieving MediaRegistry node"); + ret = -1; + goto cleanup; + } + vboxSnapshotXmlRetrieveRootNodeByName(xmlRootElement, "StorageControllers", &storageControllersNode); + if (!storageControllersNode) { + VIR_DEBUG("Problem while retrieving StorageControllers node"); + ret = -1; + goto cleanup; + } + xmlNodeDump(buf, xml, storageControllersNode, 0, 0); + if (VIR_STRDUP(snapshotStorageControllersString, (char *)xmlBufferContent(buf)) < 0) { + VIR_DEBUG("Could not duplicate %s", (char *)xmlBufferContent(buf)); + } + xmlBufferFree(buf); + if (snapshotHasParent) + numberOfDisks = def->dom->ndisks; + else + numberOfDisks = def->ndisks; + + for (i = 0; i < numberOfDisks; ++i) { + IMedium *diskSnapshot = NULL; + IMedium *baseDiskSnapshot = NULL; + PRUnichar *locationDiskSnapshotUtf16 = NULL; + PRUnichar *baseDiskIdUtf16 = NULL; + char *baseDiskId = NULL; + char *diskPath = NULL; + char *uuidToReplace = NULL; + if (snapshotHasParent) + diskPath = def->dom->disks[i]->src; + else + diskPath = def->disks[i].file; + VBOX_UTF8_TO_UTF16(diskPath, &locationDiskSnapshotUtf16); + + rc = data->vboxObj->vtbl->OpenMedium(data->vboxObj, locationDiskSnapshotUtf16, DeviceType_HardDisk, AccessMode_ReadWrite, false, &diskSnapshot); + reportInternalErrorIfNS_FAILED("cannot open medium"); + if (!diskSnapshot) { + VIR_DEBUG("Could not open %s", def->dom->disks[i]->src); + ret = -1; + goto cleanup; + } + rc = diskSnapshot->vtbl->GetBase(diskSnapshot, &baseDiskSnapshot); + reportInternalErrorIfNS_FAILED("cannot get base disk"); + if (!baseDiskSnapshot) { + VIR_DEBUG("Could not get base medium"); + ret = -1; + goto cleanup; + } + rc = baseDiskSnapshot->vtbl->GetId(baseDiskSnapshot, &baseDiskIdUtf16); + reportInternalErrorIfNS_FAILED("cannot get base disk id"); + VBOX_UTF16_TO_UTF8(baseDiskIdUtf16, &baseDiskId); + VIR_DEBUG("Removing disk %s from media registry", diskPath); + if (snapshotHasParent) { + vboxRemoveDiskFromMediaRegistryIfNoChildren(mediaRegistryNode->children, diskPath); + } else { + vboxRemoveAllDisksExceptParentFromMediaRegistry(mediaRegistryNode->children); + virSearchUuid(snapshotStorageControllersString, i+1, &uuidToReplace); + char *tmp = NULL; + tmp = virStrReplace(snapshotStorageControllersString, uuidToReplace, baseDiskId); + VIR_FREE(snapshotStorageControllersString); + if (VIR_STRDUP(snapshotStorageControllersString, tmp) < 0) { + VIR_DEBUG("Error while using strdup"); + } + VIR_FREE(tmp); + xmlParseInNodeContext(machineNode, snapshotStorageControllersString, (int)strlen(snapshotStorageControllersString), 0, &newStorageControllersNode); + } + VBOX_UTF16_FREE(locationDiskSnapshotUtf16); + VBOX_UTF16_FREE(baseDiskIdUtf16); + } + /*we remove the machine from vbox*/ + rc = vboxArrayGetWithUintArg(&machineDisks, machine, machine->vtbl->Unregister, CleanupMode_DetachAllReturnNone); + reportInternalErrorIfNS_FAILED("cannot unregister machine"); +# if VBOX_API_VERSION == 4002 + rc = machine->vtbl->Delete(machine, 0, dummyArray, &progress); +# elif VBOX_API_VERSION >= 4003 + rc = machine->vtbl->DeleteConfig(machine, 0, dummyArray, &progress); +# endif + reportInternalErrorIfNS_FAILED("cannot delete machine"); + if (progress != NULL) { + progress->vtbl->WaitForCompletion(progress, -1); + VBOX_RELEASE(progress); + } + + for (i = 0; i < numberOfDisks; ++i) { + IMedium *diskSnapshot= NULL; + PRUnichar *locationDiskSnapshotUtf16 = NULL; + IMedium *baseDiskSnapshot = NULL; + + VBOX_UTF8_TO_UTF16(def->dom->disks[i]->src, &locationDiskSnapshotUtf16); + rc = data->vboxObj->vtbl->OpenMedium(data->vboxObj, locationDiskSnapshotUtf16, DeviceType_HardDisk, AccessMode_ReadWrite, false, &diskSnapshot); + reportInternalErrorIfNS_FAILED("cannot open medium"); + rc = diskSnapshot->vtbl->GetBase(diskSnapshot, &baseDiskSnapshot); + reportInternalErrorIfNS_FAILED("cannot get disk base"); + vboxCloseDisk(dom, baseDiskSnapshot); + } + + /*we delete the xml part corresponding to the snapshot*/ + xmlUnlinkNode(snapshotToDelete); + xmlFreeNode(snapshotToDelete); + + /*We delete the part corresponding to the storagecontrollers if we need to*/ + if (numberOfDisks > 0 && !snapshotHasParent) { + xmlUnlinkNode(storageControllersNode); + xmlFreeNode(storageControllersNode); + xmlAddChild(machineNode, newStorageControllersNode); + } + + /*we save the modified xml*/ + if (virFileMakePath(mdir_name(settingsFilePathUtf8)) < 0) + virReportError(VIR_ERR_INTERNAL_ERROR, "%s" + " %s", + _("Cannot create path :"), + settingsFilePathUtf8); + if (xmlSaveFormatFileEnc(settingsFilePathUtf8, xml, "ISO-8859-1", 1) <= 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s" + " %s", + _("Cannot save xml file to:"), + settingsFilePathUtf8); + } + rc = data->vboxObj->vtbl->OpenMachine(data->vboxObj, settingsFilePath, &newMachine); + while (NS_FAILED(rc) && (rc == 0x80004005 /*NSCOM standard error*/) && failedCounter != 0) { + /*There is some random fails happening and they are not the error + *supposed to happen. + *I didn't have some solution on #vbox-dev + *but it appears that with another try it works so that's why we are trying 10 times maximum + * + */ + rc = data->vboxObj->vtbl->OpenMachine(data->vboxObj, settingsFilePath, &newMachine); + failedCounter--; + } + reportInternalErrorIfNS_FAILED("cannot open machine"); + if (newMachine) { + rc = data->vboxObj->vtbl->RegisterMachine(data->vboxObj, newMachine); + reportInternalErrorIfNS_FAILED("cannot register machine"); + } + else { + ret = -1; + goto cleanup; + } + + ret = 0; +cleanup: + vboxArrayRelease(&machineDisks); + VBOX_UTF16_FREE(settingsFilePath); + return ret; + +} +#endif +static int vboxDomainSnapshotDelete(virDomainSnapshotPtr snapshot, unsigned int flags) { @@ -8089,6 +8462,7 @@ vboxDomainSnapshotDelete(virDomainSnapshotPtr snapshot, IConsole *console = NULL; PRUint32 state; nsresult rc; + vboxArray snapChildren = VBOX_ARRAY_INITIALIZER; virCheckFlags(VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN | VIR_DOMAIN_SNAPSHOT_DELETE_METADATA_ONLY, -1); @@ -8116,6 +8490,23 @@ vboxDomainSnapshotDelete(virDomainSnapshotPtr snapshot, *to remove the node concerning the snapshot */ if (flags & VIR_DOMAIN_SNAPSHOT_DELETE_METADATA_ONLY) { + rc = vboxArrayGet(&snapChildren, snap, snap->vtbl->GetChildren); + if (NS_FAILED(rc)) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("could not get snapshot children")); + ret = -1; + goto cleanup; + } + if (snapChildren.count != 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("cannot delete metadata of a snapshot with children")); + ret = -1; + goto cleanup; + } else { +#if VBOX_API_VERSION >= 4002 + vboxDomainSnapshotDeleteMetadataOnly(snapshot); +#endif + } ret = 0; goto cleanup; } -- 1.7.10.4
On 25.11.2013 18:23, Manuel VIVES wrote:
Hi, This is a serie of patches in order to support undefining and redefining snapshots with VirtualBox 4.2.
The serie of patches is rather big, and adds among other things some utility functions unrelated to VirtualBox in patches 1 & 2. The code review could be done in several parts: e.g. patches 1 & 2 separately to validate the utility functions.
The VirtualBox API provides only high level operations to manipulate snapshots, so it not possible to support flags like VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE and VIR_DOMAIN_SNAPSHOT_DELETE_METADATA_ONLY with only API calls. Following an IRC talk with Eric Blake, the decision was taken to emulate these behaviours by manipulating directly the .vbox XML files.
The first two patches are some util methods for handling uuid and strings that will be used after.
The third patch brings more details in the snapshot XML returned by libvirt. We will need those modifications in order to redefine the snapshots.
The fourth patch brings the support of the VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE and VIR_DOMAIN_SNAPSHOT_CREATE_CURRENT flags in virDomainSnapshotCreateXML.
The fifth and last patch brings the support of the VIR_DOMAIN_SNAPSHOT_DELETE_METADATA_ONLY flag in virDomainSnapshotDelete.
The patches are only tested with Virtualbox 4.2 but the code is compliant with Virtualbox 4.3 API.
Regards, Manuel VIVES
V4: * The code is compliant with Virtualbox 4.3 API * Some minor modifications in order to satisfy "make syntax-check"
V3: * Use of STREQ_NULLABLE instead of STREQ in one case * Fix the method for finding uuids according to Ján Tomko review
V2: * Fix a licence problem with the method for string replacement
Manuel VIVES (5): viruuid.h/c: Util method for finding uuid patterns in some strings virstring.h/c: Util method for making some find and replace in strings vbox_tmpl.c: Better XML description for snapshots vbox_tmpl.c: Patch for redefining snapshots vbox_tmpl.c: Add methods for undefining snapshots
po/POTFILES.in | 1 + src/conf/domain_conf.c | 2 +- src/libvirt_private.syms | 2 + src/util/virstring.c | 48 ++ src/util/virstring.h | 2 + src/util/viruuid.c | 81 ++ src/util/viruuid.h | 1 + src/vbox/vbox_tmpl.c | 1854 +++++++++++++++++++++++++++++++++++++++++++--- 8 files changed, 1879 insertions(+), 112 deletions(-)
Can you rebase and resend please? I was about to review this (better late than never, right?) but the HEAD moved a lot since then and I'm unable to apply the patches cleanly. Michal
participants (3)
-
Laine Stump -
Manuel VIVES -
Michal Privoznik