[libvirt] [PATCH 0/4] Make rewriting XML files more robust
Jiri Denemark (4): Introduce virFileSync Introduce virFileRewrite for safe file rewrite Introduce virXMLSaveFile as a wrapper for virFileRewrite Use virXMLSaveFile when writing XML config src/conf/domain_conf.c | 32 +------------------- src/conf/network_conf.c | 34 +-------------------- src/conf/nwfilter_conf.c | 68 ++---------------------------------------- src/conf/storage_conf.c | 37 ++--------------------- src/libvirt_private.syms | 3 ++ src/qemu/qemu_domain.c | 19 +----------- src/util/util.c | 4 ++- src/util/virfile.c | 72 ++++++++++++++++++++++++++++++++++++++++++++++ src/util/virfile.h | 8 +++++ src/util/xml.c | 36 +++++++++++++++++++++++ src/util/xml.h | 5 +++ 11 files changed, 141 insertions(+), 177 deletions(-) -- 1.7.7
It still needs to be enhanced to be fully portable and working even if fsync() implementation is insane (e.g., on OSX). --- Notes: Although ideally gnulib would provide us with a sane fsync() and fdatasync() replacements (it already does so for fsync on Win32) so this could probably be dropped in favor of a possible future extension of gnulib (we do not provide any better solution anyway). src/libvirt_private.syms | 1 + src/util/virfile.c | 16 ++++++++++++++++ src/util/virfile.h | 2 ++ 3 files changed, 19 insertions(+), 0 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index dedbd16..9666a0a 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1172,6 +1172,7 @@ virFileDirectFdFree; virFileDirectFdNew; virFileFclose; virFileFdopen; +virFileSync; # virnetmessage.h diff --git a/src/util/virfile.c b/src/util/virfile.c index 1158998..50b8aab 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -334,3 +334,19 @@ int virFileUnlock(int fd ATTRIBUTE_UNUSED, return -ENOSYS; } #endif + +int +virFileSync(int fd, bool metadata) +{ + int ret; + + /* XXX This needs to be enhanced to properly sync even on OSX which + * lacks sane fsync() */ + + if (metadata) + ret = fsync(fd); + else + ret = fdatasync(fd); + + return ret; +} diff --git a/src/util/virfile.h b/src/util/virfile.h index e025614..0b14e1d 100644 --- a/src/util/virfile.h +++ b/src/util/virfile.h @@ -68,4 +68,6 @@ void virFileDirectFdFree(virFileDirectFdPtr dfd); int virFileLock(int fd, bool shared, off_t start, off_t len); int virFileUnlock(int fd, off_t start, off_t len); +int virFileSync(int fd, bool metadata); + #endif /* __VIR_FILES_H */ -- 1.7.7
On Wed, Oct 19, 2011 at 07:26:24PM +0200, Jiri Denemark wrote:
It still needs to be enhanced to be fully portable and working even if fsync() implementation is insane (e.g., on OSX). --- Notes: Although ideally gnulib would provide us with a sane fsync() and fdatasync() replacements (it already does so for fsync on Win32) so this could probably be dropped in favor of a possible future extension of gnulib (we do not provide any better solution anyway).
src/libvirt_private.syms | 1 + src/util/virfile.c | 16 ++++++++++++++++ src/util/virfile.h | 2 ++ 3 files changed, 19 insertions(+), 0 deletions(-)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index dedbd16..9666a0a 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1172,6 +1172,7 @@ virFileDirectFdFree; virFileDirectFdNew; virFileFclose; virFileFdopen; +virFileSync;
# virnetmessage.h diff --git a/src/util/virfile.c b/src/util/virfile.c index 1158998..50b8aab 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -334,3 +334,19 @@ int virFileUnlock(int fd ATTRIBUTE_UNUSED, return -ENOSYS; } #endif + +int +virFileSync(int fd, bool metadata) +{ + int ret; + + /* XXX This needs to be enhanced to properly sync even on OSX which + * lacks sane fsync() */ + + if (metadata) + ret = fsync(fd); + else + ret = fdatasync(fd); + + return ret; +} diff --git a/src/util/virfile.h b/src/util/virfile.h index e025614..0b14e1d 100644 --- a/src/util/virfile.h +++ b/src/util/virfile.h @@ -68,4 +68,6 @@ void virFileDirectFdFree(virFileDirectFdPtr dfd); int virFileLock(int fd, bool shared, off_t start, off_t len); int virFileUnlock(int fd, off_t start, off_t len);
+int virFileSync(int fd, bool metadata); + #endif /* __VIR_FILES_H */
ACK Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On Wed, Oct 19, 2011 at 19:26:24 +0200, Jiri Denemark wrote:
It still needs to be enhanced to be fully portable and working even if fsync() implementation is insane (e.g., on OSX). --- Notes: Although ideally gnulib would provide us with a sane fsync() and fdatasync() replacements (it already does so for fsync on Win32) so this could probably be dropped in favor of a possible future extension of gnulib (we do not provide any better solution anyway).
I will just drop this patch. Gnulib now provides fdatasync replacement and as it turned out fsync on OSX is not as bad as we thought so we don't need a replacement there. virFileSync doesn't give us anything beyond that. Jirka
When saving config files we just overwrite old content of the file. In case something fails during that process (e.g. disk gets full) we lose both old and new content. This patch makes the process more robust by writing the new content into a separate file and only if that succeeds the original file is atomically replaced with the new one. --- src/libvirt_private.syms | 1 + src/util/virfile.c | 56 ++++++++++++++++++++++++++++++++++++++++++++++ src/util/virfile.h | 6 +++++ 3 files changed, 63 insertions(+), 0 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 9666a0a..1c7910b 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1172,6 +1172,7 @@ virFileDirectFdFree; virFileDirectFdNew; virFileFclose; virFileFdopen; +virFileRewrite; virFileSync; diff --git a/src/util/virfile.c b/src/util/virfile.c index 50b8aab..8c00e86 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -350,3 +350,59 @@ virFileSync(int fd, bool metadata) return ret; } + +int +virFileRewrite(const char *path, + mode_t mode, + virFileRewriteFunc rewrite, + void *opaque) +{ + char *newfile = NULL; + int fd = -1; + int ret = -1; + + if (virAsprintf(&newfile, "%s.new", path) < 0) { + virReportOOMError(); + goto cleanup; + } + + if ((fd = open(newfile, O_WRONLY | O_CREAT | O_TRUNC, mode)) < 0) { + virReportSystemError(errno, _("cannot create file '%s'"), + newfile); + goto cleanup; + } + + if (rewrite(fd, opaque) < 0) { + virReportSystemError(errno, _("cannot write data to file '%s'"), + newfile); + goto cleanup; + } + + if (virFileSync(fd, true) < 0) { + virReportSystemError(errno, _("cannot sync file '%s'"), + newfile); + goto cleanup; + } + + if (VIR_CLOSE(fd) < 0) { + virReportSystemError(errno, _("cannot save file '%s'"), + newfile); + goto cleanup; + } + + if (rename(newfile, path) < 0) { + virReportSystemError(errno, _("cannot rename file '%s' as '%s'"), + newfile, path); + goto cleanup; + } + + ret = 0; + +cleanup: + VIR_FORCE_CLOSE(fd); + if (newfile) { + unlink(newfile); + VIR_FREE(newfile); + } + return ret; +} diff --git a/src/util/virfile.h b/src/util/virfile.h index 0b14e1d..c26ff5a 100644 --- a/src/util/virfile.h +++ b/src/util/virfile.h @@ -70,4 +70,10 @@ int virFileUnlock(int fd, off_t start, off_t len); int virFileSync(int fd, bool metadata); +typedef int (*virFileRewriteFunc)(int fd, void *opaque); +int virFileRewrite(const char *path, + mode_t mode, + virFileRewriteFunc rewrite, + void *opaque); + #endif /* __VIR_FILES_H */ -- 1.7.7
On Wed, Oct 19, 2011 at 07:26:25PM +0200, Jiri Denemark wrote:
When saving config files we just overwrite old content of the file. In case something fails during that process (e.g. disk gets full) we lose both old and new content. This patch makes the process more robust by writing the new content into a separate file and only if that succeeds the original file is atomically replaced with the new one. --- src/libvirt_private.syms | 1 + src/util/virfile.c | 56 ++++++++++++++++++++++++++++++++++++++++++++++ src/util/virfile.h | 6 +++++ 3 files changed, 63 insertions(+), 0 deletions(-)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 9666a0a..1c7910b 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1172,6 +1172,7 @@ virFileDirectFdFree; virFileDirectFdNew; virFileFclose; virFileFdopen; +virFileRewrite; virFileSync;
diff --git a/src/util/virfile.c b/src/util/virfile.c index 50b8aab..8c00e86 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -350,3 +350,59 @@ virFileSync(int fd, bool metadata)
return ret; } + +int +virFileRewrite(const char *path, + mode_t mode, + virFileRewriteFunc rewrite, + void *opaque) +{ + char *newfile = NULL; + int fd = -1; + int ret = -1; + + if (virAsprintf(&newfile, "%s.new", path) < 0) { + virReportOOMError(); + goto cleanup; + } + + if ((fd = open(newfile, O_WRONLY | O_CREAT | O_TRUNC, mode)) < 0) { + virReportSystemError(errno, _("cannot create file '%s'"), + newfile); + goto cleanup; + } + + if (rewrite(fd, opaque) < 0) { + virReportSystemError(errno, _("cannot write data to file '%s'"), + newfile); + goto cleanup; + } + + if (virFileSync(fd, true) < 0) { + virReportSystemError(errno, _("cannot sync file '%s'"), + newfile); + goto cleanup; + } + + if (VIR_CLOSE(fd) < 0) { + virReportSystemError(errno, _("cannot save file '%s'"), + newfile); + goto cleanup; + } + + if (rename(newfile, path) < 0) { + virReportSystemError(errno, _("cannot rename file '%s' as '%s'"), + newfile, path); + goto cleanup; + } + + ret = 0; + +cleanup: + VIR_FORCE_CLOSE(fd); + if (newfile) { + unlink(newfile); + VIR_FREE(newfile); + } + return ret; +} diff --git a/src/util/virfile.h b/src/util/virfile.h index 0b14e1d..c26ff5a 100644 --- a/src/util/virfile.h +++ b/src/util/virfile.h @@ -70,4 +70,10 @@ int virFileUnlock(int fd, off_t start, off_t len);
int virFileSync(int fd, bool metadata);
+typedef int (*virFileRewriteFunc)(int fd, void *opaque); +int virFileRewrite(const char *path, + mode_t mode, + virFileRewriteFunc rewrite, + void *opaque); + #endif /* __VIR_FILES_H */
ACK Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 10/19/2011 11:26 AM, Jiri Denemark wrote:
When saving config files we just overwrite old content of the file. In case something fails during that process (e.g. disk gets full) we lose both old and new content. This patch makes the process more robust by writing the new content into a separate file and only if that succeeds the original file is atomically replaced with the new one. --- src/libvirt_private.syms | 1 + src/util/virfile.c | 56 ++++++++++++++++++++++++++++++++++++++++++++++ src/util/virfile.h | 6 +++++ 3 files changed, 63 insertions(+), 0 deletions(-)
+virFileRewrite(const char *path, + mode_t mode, + virFileRewriteFunc rewrite, + void *opaque) +{ + char *newfile = NULL; + int fd = -1; + int ret = -1; + + if (virAsprintf(&newfile, "%s.new", path)< 0) { + virReportOOMError(); + goto cleanup; + } + + if ((fd = open(newfile, O_WRONLY | O_CREAT | O_TRUNC, mode))< 0) {
Should we be using O_EXCL instead of O_TRUNC, and insisting that no .new file already exists, so as to protect ourselves from symlink attacks where someone installs a symlink and tricks us into truncating a random file?
+ + if (rename(newfile, path)< 0) { + virReportSystemError(errno, _("cannot rename file '%s' as '%s'"), + newfile, path); + goto cleanup; + } + + ret = 0; + +cleanup: + VIR_FORCE_CLOSE(fd); + if (newfile) { + unlink(newfile);
This fails if the rename() succeeds, or worse, it succeeds at unlinking an unrelated file that someone created in the window between our rename and this unlink. We should probably only attempt the unlink if ret < 0. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On Mon, Oct 24, 2011 at 16:48:58 -0600, Eric Blake wrote:
On 10/19/2011 11:26 AM, Jiri Denemark wrote:
When saving config files we just overwrite old content of the file. In case something fails during that process (e.g. disk gets full) we lose both old and new content. This patch makes the process more robust by writing the new content into a separate file and only if that succeeds the original file is atomically replaced with the new one. --- src/libvirt_private.syms | 1 + src/util/virfile.c | 56 ++++++++++++++++++++++++++++++++++++++++++++++ src/util/virfile.h | 6 +++++ 3 files changed, 63 insertions(+), 0 deletions(-)
+virFileRewrite(const char *path, + mode_t mode, + virFileRewriteFunc rewrite, + void *opaque) +{ + char *newfile = NULL; + int fd = -1; + int ret = -1; + + if (virAsprintf(&newfile, "%s.new", path)< 0) { + virReportOOMError(); + goto cleanup; + } + + if ((fd = open(newfile, O_WRONLY | O_CREAT | O_TRUNC, mode))< 0) {
Should we be using O_EXCL instead of O_TRUNC, and insisting that no .new file already exists, so as to protect ourselves from symlink attacks where someone installs a symlink and tricks us into truncating a random file?
I think this would need to be configurable and can be safely deferred to the future when there is a need to use this API for other purposes than writing libvirt's XML files. In this case we don't care about existing .new files and it's much easier for the user running libvirtd to replace a random file than trick libvirtd to do it.
+ + if (rename(newfile, path)< 0) { + virReportSystemError(errno, _("cannot rename file '%s' as '%s'"), + newfile, path); + goto cleanup; + } + + ret = 0; + +cleanup: + VIR_FORCE_CLOSE(fd); + if (newfile) { + unlink(newfile);
This fails if the rename() succeeds, or worse, it succeeds at unlinking an unrelated file that someone created in the window between our rename and this unlink. We should probably only attempt the unlink if ret < 0.
OK, although someone else could have unlinked and created the same file between our open and unlink, too :-) Note that none of these situation can happen since we only use this function to save libvirt's XML files and only one thread can create a file with the same name. Jirka
On 10/26/2011 03:16 PM, Jiri Denemark wrote:
On Mon, Oct 24, 2011 at 16:48:58 -0600, Eric Blake wrote:
On 10/19/2011 11:26 AM, Jiri Denemark wrote:
When saving config files we just overwrite old content of the file. In case something fails during that process (e.g. disk gets full) we lose both old and new content. This patch makes the process more robust by writing the new content into a separate file and only if that succeeds the original file is atomically replaced with the new one. ---
+ if ((fd = open(newfile, O_WRONLY | O_CREAT | O_TRUNC, mode))< 0) {
Should we be using O_EXCL instead of O_TRUNC, and insisting that no .new file already exists, so as to protect ourselves from symlink attacks where someone installs a symlink and tricks us into truncating a random file?
I think this would need to be configurable and can be safely deferred to the future when there is a need to use this API for other purposes than writing libvirt's XML files. In this case we don't care about existing .new files and it's much easier for the user running libvirtd to replace a random file than trick libvirtd to do it.
On the other hand, while the lack of O_EXCL isn't as robust as it could be, if someone already has enough write permissions to inject a symlink attack into our libvirt-internal directory with .new files, they already have enough permissions to do a lot of other damage, too. I'm okay with pushing this as-is for now, and worrying about further improvements with O_EXCL at a later date, if we ever use this function in more than just libvirt-internal directories. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
Every time we write XML into a file we call virEmitXMLWarning to write a warning that the file is automatically generated. virXMLSaveFile simplifies this into a single step and makes rewriting existing XML file safe by using virFileRewrite internally. --- src/conf/domain_conf.c | 25 +++++++++++++++++++++---- src/libvirt_private.syms | 1 + src/util/util.c | 4 +++- src/util/xml.c | 36 ++++++++++++++++++++++++++++++++++++ src/util/xml.h | 5 +++++ 5 files changed, 66 insertions(+), 5 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 5959593..6656e8b 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -11078,6 +11078,7 @@ int virDomainSaveXML(const char *configDir, const char *xml) { char *configFile = NULL; + char *newfile = NULL; int fd = -1, ret = -1; size_t towrite; @@ -11091,12 +11092,17 @@ int virDomainSaveXML(const char *configDir, goto cleanup; } - if ((fd = open(configFile, + if (virAsprintf(&newfile, "%s.new", configFile) < 0) { + virReportOOMError(); + goto cleanup; + } + + if ((fd = open(newfile, O_WRONLY | O_CREAT | O_TRUNC, S_IRUSR | S_IWUSR )) < 0) { virReportSystemError(errno, _("cannot create config file '%s'"), - configFile); + newfile); goto cleanup; } @@ -11106,14 +11112,21 @@ int virDomainSaveXML(const char *configDir, if (safewrite(fd, xml, towrite) < 0) { virReportSystemError(errno, _("cannot write config file '%s'"), - configFile); + newfile); goto cleanup; } if (VIR_CLOSE(fd) < 0) { virReportSystemError(errno, _("cannot save config file '%s'"), - configFile); + newfile); + goto cleanup; + } + + if (rename(newfile, configFile) < 0) { + virReportSystemError(errno, + _("cannot rename config file '%s' as '%s'"), + newfile, configFile); goto cleanup; } @@ -11121,6 +11134,10 @@ int virDomainSaveXML(const char *configDir, cleanup: VIR_FORCE_CLOSE(fd); + if (newfile) { + unlink(newfile); + VIR_FREE(newfile); + } VIR_FREE(configFile); return ret; } diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 1c7910b..b05bf61 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1292,6 +1292,7 @@ virKeycodeValueTranslate; # xml.h virXMLParseHelper; virXMLPropString; +virXMLSaveFile; virXPathBoolean; virXPathInt; virXPathLong; diff --git a/src/util/util.c b/src/util/util.c index dac616b..18763b1 100644 --- a/src/util/util.c +++ b/src/util/util.c @@ -2485,8 +2485,10 @@ OVERWRITTEN AND LOST. Changes to this xml configuration should be made using:\n\ or other application using the libvirt API.\n\ -->\n\n"; - if (fd < 0 || !name || !cmd) + if (fd < 0 || !name || !cmd) { + errno = EINVAL; return -1; + } len = strlen(prologue); if (safewrite(fd, prologue, len) != len) diff --git a/src/util/xml.c b/src/util/xml.c index b0942da..1ff728e 100644 --- a/src/util/xml.c +++ b/src/util/xml.c @@ -16,12 +16,14 @@ #include <stdarg.h> #include <limits.h> #include <math.h> /* for isnan() */ +#include <sys/stat.h> #include "virterror_internal.h" #include "xml.h" #include "buf.h" #include "util.h" #include "memory.h" +#include "virfile.h" #define VIR_FROM_THIS VIR_FROM_XML @@ -797,3 +799,37 @@ error: } goto cleanup; } + + +struct rewrite_data { + const char *warnName; + const char *warnCommand; + const char *xml; +}; + +static int +virXMLRewriteFile(int fd, void *opaque) +{ + struct rewrite_data *data = opaque; + + if (data->warnName && data->warnCommand) { + if (virEmitXMLWarning(fd, data->warnName, data->warnCommand) < 0) + return -1; + } + + if (safewrite(fd, data->xml, strlen(data->xml)) < 0) + return -1; + + return 0; +} + +int +virXMLSaveFile(const char *path, + const char *warnName, + const char *warnCommand, + const char *xml) +{ + struct rewrite_data data = { warnName, warnCommand, xml }; + + return virFileRewrite(path, S_IRUSR | S_IWUSR, virXMLRewriteFile, &data); +} diff --git a/src/util/xml.h b/src/util/xml.h index d30e066..c492063 100644 --- a/src/util/xml.h +++ b/src/util/xml.h @@ -138,4 +138,9 @@ xmlDocPtr virXMLParseHelper(int domcode, # define virXMLParseFileCtxt(filename, pctxt) \ virXMLParseHelper(VIR_FROM_THIS, filename, NULL, NULL, pctxt) +int virXMLSaveFile(const char *path, + const char *warnName, + const char *warnCommand, + const char *xml); + #endif /* __VIR_XML_H__ */ -- 1.7.7
On Wed, Oct 19, 2011 at 07:26:26PM +0200, Jiri Denemark wrote:
Every time we write XML into a file we call virEmitXMLWarning to write a warning that the file is automatically generated. virXMLSaveFile simplifies this into a single step and makes rewriting existing XML file safe by using virFileRewrite internally. --- src/conf/domain_conf.c | 25 +++++++++++++++++++++---- src/libvirt_private.syms | 1 + src/util/util.c | 4 +++- src/util/xml.c | 36 ++++++++++++++++++++++++++++++++++++ src/util/xml.h | 5 +++++ 5 files changed, 66 insertions(+), 5 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 5959593..6656e8b 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -11078,6 +11078,7 @@ int virDomainSaveXML(const char *configDir, const char *xml) { char *configFile = NULL; + char *newfile = NULL; int fd = -1, ret = -1; size_t towrite;
@@ -11091,12 +11092,17 @@ int virDomainSaveXML(const char *configDir, goto cleanup; }
- if ((fd = open(configFile, + if (virAsprintf(&newfile, "%s.new", configFile) < 0) { + virReportOOMError(); + goto cleanup; + } + + if ((fd = open(newfile, O_WRONLY | O_CREAT | O_TRUNC, S_IRUSR | S_IWUSR )) < 0) { virReportSystemError(errno, _("cannot create config file '%s'"), - configFile); + newfile); goto cleanup; }
@@ -11106,14 +11112,21 @@ int virDomainSaveXML(const char *configDir, if (safewrite(fd, xml, towrite) < 0) { virReportSystemError(errno, _("cannot write config file '%s'"), - configFile); + newfile); goto cleanup; }
if (VIR_CLOSE(fd) < 0) { virReportSystemError(errno, _("cannot save config file '%s'"), - configFile); + newfile); + goto cleanup; + } + + if (rename(newfile, configFile) < 0) { + virReportSystemError(errno, + _("cannot rename config file '%s' as '%s'"), + newfile, configFile); goto cleanup; }
@@ -11121,6 +11134,10 @@ int virDomainSaveXML(const char *configDir, cleanup: VIR_FORCE_CLOSE(fd);
+ if (newfile) { + unlink(newfile); + VIR_FREE(newfile); + } VIR_FREE(configFile); return ret; }
Since this hunk is obliterated by the next patch, I'm thinking you perhaps forgot to squash this into patch 4 before posting ?
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 1c7910b..b05bf61 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1292,6 +1292,7 @@ virKeycodeValueTranslate; # xml.h virXMLParseHelper; virXMLPropString; +virXMLSaveFile; virXPathBoolean; virXPathInt; virXPathLong;
diff --git a/src/util/xml.c b/src/util/xml.c index b0942da..1ff728e 100644 --- a/src/util/xml.c +++ b/src/util/xml.c @@ -16,12 +16,14 @@ #include <stdarg.h> #include <limits.h> #include <math.h> /* for isnan() */ +#include <sys/stat.h>
#include "virterror_internal.h" #include "xml.h" #include "buf.h" #include "util.h" #include "memory.h" +#include "virfile.h"
#define VIR_FROM_THIS VIR_FROM_XML
@@ -797,3 +799,37 @@ error: } goto cleanup; } + + +struct rewrite_data {
s/rewrite_data/virXMLRewritFileData/
+ const char *warnName; + const char *warnCommand; + const char *xml; +}; + +static int +virXMLRewriteFile(int fd, void *opaque) +{ + struct rewrite_data *data = opaque; + + if (data->warnName && data->warnCommand) { + if (virEmitXMLWarning(fd, data->warnName, data->warnCommand) < 0) + return -1; + } + + if (safewrite(fd, data->xml, strlen(data->xml)) < 0) + return -1; + + return 0; +} + +int +virXMLSaveFile(const char *path, + const char *warnName, + const char *warnCommand, + const char *xml) +{ + struct rewrite_data data = { warnName, warnCommand, xml }; + + return virFileRewrite(path, S_IRUSR | S_IWUSR, virXMLRewriteFile, &data); +} diff --git a/src/util/xml.h b/src/util/xml.h index d30e066..c492063 100644 --- a/src/util/xml.h +++ b/src/util/xml.h @@ -138,4 +138,9 @@ xmlDocPtr virXMLParseHelper(int domcode, # define virXMLParseFileCtxt(filename, pctxt) \ virXMLParseHelper(VIR_FROM_THIS, filename, NULL, NULL, pctxt)
+int virXMLSaveFile(const char *path, + const char *warnName, + const char *warnCommand, + const char *xml); + #endif /* __VIR_XML_H__ */
ACK with those small changes Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 10/19/2011 11:26 AM, Jiri Denemark wrote:
Every time we write XML into a file we call virEmitXMLWarning to write a warning that the file is automatically generated. virXMLSaveFile simplifies this into a single step and makes rewriting existing XML file safe by using virFileRewrite internally. --- src/conf/domain_conf.c | 25 +++++++++++++++++++++---- src/libvirt_private.syms | 1 + src/util/util.c | 4 +++- src/util/xml.c | 36 ++++++++++++++++++++++++++++++++++++ src/util/xml.h | 5 +++++ 5 files changed, 66 insertions(+), 5 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
- if ((fd = open(configFile, + if (virAsprintf(&newfile, "%s.new", configFile)< 0) { + virReportOOMError(); + goto cleanup; + } + + if ((fd = open(newfile, O_WRONLY | O_CREAT | O_TRUNC, S_IRUSR | S_IWUSR ))< 0) {
Same question about O_TRUNC vs. O_EXCL.
@@ -11121,6 +11134,10 @@ int virDomainSaveXML(const char *configDir, cleanup: VIR_FORCE_CLOSE(fd);
+ if (newfile) { + unlink(newfile);
Same concern about blind unlink(). -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On Mon, Oct 24, 2011 at 16:51:49 -0600, Eric Blake wrote:
On 10/19/2011 11:26 AM, Jiri Denemark wrote:
Every time we write XML into a file we call virEmitXMLWarning to write a warning that the file is automatically generated. virXMLSaveFile simplifies this into a single step and makes rewriting existing XML file safe by using virFileRewrite internally. --- src/conf/domain_conf.c | 25 +++++++++++++++++++++---- src/libvirt_private.syms | 1 + src/util/util.c | 4 +++- src/util/xml.c | 36 ++++++++++++++++++++++++++++++++++++ src/util/xml.h | 5 +++++ 5 files changed, 66 insertions(+), 5 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
- if ((fd = open(configFile, + if (virAsprintf(&newfile, "%s.new", configFile)< 0) { + virReportOOMError(); + goto cleanup; + } + + if ((fd = open(newfile, O_WRONLY | O_CREAT | O_TRUNC, S_IRUSR | S_IWUSR ))< 0) {
Same question about O_TRUNC vs. O_EXCL.
@@ -11121,6 +11134,10 @@ int virDomainSaveXML(const char *configDir, cleanup: VIR_FORCE_CLOSE(fd);
+ if (newfile) { + unlink(newfile);
Same concern about blind unlink().
As mentioned by Daniel, these hunks shouldn't be here. In fact, this patch shouldn't touch src/conf/domain_conf.c at all and the changes to that file should be squashed into the next patch (which effectively deletes all the changes). Sorry for the mess. Jirka
--- src/conf/domain_conf.c | 49 +------------------------------- src/conf/network_conf.c | 34 +--------------------- src/conf/nwfilter_conf.c | 68 +++------------------------------------------- src/conf/storage_conf.c | 37 +++---------------------- src/qemu/qemu_domain.c | 19 +----------- 5 files changed, 14 insertions(+), 193 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 6656e8b..dda5e1a 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -11078,9 +11078,7 @@ int virDomainSaveXML(const char *configDir, const char *xml) { char *configFile = NULL; - char *newfile = NULL; - int fd = -1, ret = -1; - size_t towrite; + int ret = -1; if ((configFile = virDomainConfigFile(configDir, def->name)) == NULL) goto cleanup; @@ -11092,52 +11090,9 @@ int virDomainSaveXML(const char *configDir, goto cleanup; } - if (virAsprintf(&newfile, "%s.new", configFile) < 0) { - virReportOOMError(); - goto cleanup; - } - - if ((fd = open(newfile, - O_WRONLY | O_CREAT | O_TRUNC, - S_IRUSR | S_IWUSR )) < 0) { - virReportSystemError(errno, - _("cannot create config file '%s'"), - newfile); - goto cleanup; - } - - virEmitXMLWarning(fd, def->name, "edit"); - - towrite = strlen(xml); - if (safewrite(fd, xml, towrite) < 0) { - virReportSystemError(errno, - _("cannot write config file '%s'"), - newfile); - goto cleanup; - } - - if (VIR_CLOSE(fd) < 0) { - virReportSystemError(errno, - _("cannot save config file '%s'"), - newfile); - goto cleanup; - } - - if (rename(newfile, configFile) < 0) { - virReportSystemError(errno, - _("cannot rename config file '%s' as '%s'"), - newfile, configFile); - goto cleanup; - } + ret = virXMLSaveFile(configFile, def->name, "edit", xml); - ret = 0; cleanup: - VIR_FORCE_CLOSE(fd); - - if (newfile) { - unlink(newfile); - VIR_FREE(newfile); - } VIR_FREE(configFile); return ret; } diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index b98ffad..f2ea9bf 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -1389,8 +1389,7 @@ int virNetworkSaveXML(const char *configDir, const char *xml) { char *configFile = NULL; - int fd = -1, ret = -1; - size_t towrite; + int ret = -1; if ((configFile = virNetworkConfigFile(configDir, def->name)) == NULL) goto cleanup; @@ -1402,39 +1401,10 @@ int virNetworkSaveXML(const char *configDir, goto cleanup; } - if ((fd = open(configFile, - O_WRONLY | O_CREAT | O_TRUNC, - S_IRUSR | S_IWUSR )) < 0) { - virReportSystemError(errno, - _("cannot create config file '%s'"), - configFile); - goto cleanup; - } - - virEmitXMLWarning(fd, def->name, "net-edit"); - - towrite = strlen(xml); - if (safewrite(fd, xml, towrite) < 0) { - virReportSystemError(errno, - _("cannot write config file '%s'"), - configFile); - goto cleanup; - } - - if (VIR_CLOSE(fd) < 0) { - virReportSystemError(errno, - _("cannot save config file '%s'"), - configFile); - goto cleanup; - } - - ret = 0; + ret = virXMLSaveFile(configFile, def->name, "net-edit", xml); cleanup: - VIR_FORCE_CLOSE(fd); - VIR_FREE(configFile); - return ret; } diff --git a/src/conf/nwfilter_conf.c b/src/conf/nwfilter_conf.c index 08ede48..5527348 100644 --- a/src/conf/nwfilter_conf.c +++ b/src/conf/nwfilter_conf.c @@ -2182,8 +2182,7 @@ int virNWFilterSaveXML(const char *configDir, const char *xml) { char *configFile = NULL; - int fd = -1, ret = -1; - size_t towrite; + int ret = -1; if ((configFile = virNWFilterConfigFile(configDir, def->name)) == NULL) goto cleanup; @@ -2195,38 +2194,10 @@ int virNWFilterSaveXML(const char *configDir, goto cleanup; } - if ((fd = open(configFile, - O_WRONLY | O_CREAT | O_TRUNC, - S_IRUSR | S_IWUSR )) < 0) { - virReportSystemError(errno, - _("cannot create config file '%s'"), - configFile); - goto cleanup; - } - - virEmitXMLWarning(fd, def->name, "nwfilter-edit"); - - towrite = strlen(xml); - if (safewrite(fd, xml, towrite) < 0) { - virReportSystemError(errno, - _("cannot write config file '%s'"), - configFile); - goto cleanup; - } - - if (VIR_CLOSE(fd) < 0) { - virReportSystemError(errno, - _("cannot save config file '%s'"), - configFile); - goto cleanup; - } - - ret = 0; + ret = virXMLSaveFile(configFile, def->name, "nwfilter-edit", xml); cleanup: - VIR_FORCE_CLOSE(fd); VIR_FREE(configFile); - return ret; } @@ -2569,8 +2540,7 @@ virNWFilterObjSaveDef(virNWFilterDriverStatePtr driver, virNWFilterDefPtr def) { char *xml; - int fd = -1, ret = -1; - ssize_t towrite; + int ret; if (!nwfilter->configFile) { if (virFileMakePath(driver->configDir) < 0) { @@ -2592,37 +2562,7 @@ virNWFilterObjSaveDef(virNWFilterDriverStatePtr driver, return -1; } - if ((fd = open(nwfilter->configFile, - O_WRONLY | O_CREAT | O_TRUNC, - S_IRUSR | S_IWUSR )) < 0) { - virReportSystemError(errno, - _("cannot create config file %s"), - nwfilter->configFile); - goto cleanup; - } - - virEmitXMLWarning(fd, def->name, "nwfilter-edit"); - - towrite = strlen(xml); - if (safewrite(fd, xml, towrite) != towrite) { - virReportSystemError(errno, - _("cannot write config file %s"), - nwfilter->configFile); - goto cleanup; - } - - if (VIR_CLOSE(fd) < 0) { - virReportSystemError(errno, - _("cannot save config file %s"), - nwfilter->configFile); - goto cleanup; - } - - ret = 0; - - cleanup: - VIR_FORCE_CLOSE(fd); - + ret = virXMLSaveFile(nwfilter->configFile, def->name, "nwfilter-edit", xml); VIR_FREE(xml); return ret; diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index e893b2d..36b24da 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -1515,10 +1515,10 @@ virStoragePoolLoadAllConfigs(virStoragePoolObjListPtr pools, int virStoragePoolObjSaveDef(virStorageDriverStatePtr driver, virStoragePoolObjPtr pool, - virStoragePoolDefPtr def) { + virStoragePoolDefPtr def) +{ char *xml; - int fd = -1, ret = -1; - ssize_t towrite; + int ret = -1; if (!pool->configFile) { if (virFileMakePath(driver->configDir) < 0) { @@ -1546,36 +1546,7 @@ virStoragePoolObjSaveDef(virStorageDriverStatePtr driver, return -1; } - if ((fd = open(pool->configFile, - O_WRONLY | O_CREAT | O_TRUNC, - S_IRUSR | S_IWUSR )) < 0) { - virReportSystemError(errno, - _("cannot create config file %s"), - pool->configFile); - goto cleanup; - } - - virEmitXMLWarning(fd, def->name, "pool-edit"); - - towrite = strlen(xml); - if (safewrite(fd, xml, towrite) != towrite) { - virReportSystemError(errno, - _("cannot write config file %s"), - pool->configFile); - goto cleanup; - } - - if (VIR_CLOSE(fd) < 0) { - virReportSystemError(errno, - _("cannot save config file %s"), - pool->configFile); - goto cleanup; - } - - ret = 0; - - cleanup: - VIR_FORCE_CLOSE(fd); + ret = virXMLSaveFile(pool->configFile, def->name, "pool-edit", xml); VIR_FREE(xml); return ret; diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index b202ba7..7fa8523 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -1336,7 +1336,6 @@ qemuDomainSnapshotWriteMetadata(virDomainObjPtr vm, virDomainSnapshotObjPtr snapshot, char *snapshotDir) { - int fd = -1; char *newxml = NULL; int ret = -1; char *snapDir = NULL; @@ -1366,33 +1365,19 @@ qemuDomainSnapshotWriteMetadata(virDomainObjPtr vm, virReportOOMError(); goto cleanup; } - fd = open(snapFile, O_CREAT|O_TRUNC|O_WRONLY, S_IRUSR|S_IWUSR); - if (fd < 0) { - qemuReportError(VIR_ERR_OPERATION_FAILED, - _("failed to create snapshot file '%s'"), snapFile); - goto cleanup; - } if (virAsprintf(&tmp, "snapshot-edit %s", vm->def->name) < 0) { virReportOOMError(); goto cleanup; } - virEmitXMLWarning(fd, snapshot->def->name, tmp); - VIR_FREE(tmp); - - if (safewrite(fd, newxml, strlen(newxml)) != strlen(newxml)) { - virReportSystemError(errno, _("Failed to write snapshot data to %s"), - snapFile); - goto cleanup; - } - ret = 0; + ret = virXMLSaveFile(snapFile, snapshot->def->name, tmp, newxml); + VIR_FREE(tmp); cleanup: VIR_FREE(snapFile); VIR_FREE(snapDir); VIR_FREE(newxml); - VIR_FORCE_CLOSE(fd); return ret; } -- 1.7.7
On Wed, Oct 19, 2011 at 07:26:27PM +0200, Jiri Denemark wrote:
--- src/conf/domain_conf.c | 49 +------------------------------- src/conf/network_conf.c | 34 +--------------------- src/conf/nwfilter_conf.c | 68 +++------------------------------------------- src/conf/storage_conf.c | 37 +++---------------------- src/qemu/qemu_domain.c | 19 +----------- 5 files changed, 14 insertions(+), 193 deletions(-)
ACK Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On Wed, Oct 19, 2011 at 19:26:23 +0200, Jiri Denemark wrote:
Jiri Denemark (4): Introduce virFileSync Introduce virFileRewrite for safe file rewrite Introduce virXMLSaveFile as a wrapper for virFileRewrite Use virXMLSaveFile when writing XML config
src/conf/domain_conf.c | 32 +------------------- src/conf/network_conf.c | 34 +-------------------- src/conf/nwfilter_conf.c | 68 ++---------------------------------------- src/conf/storage_conf.c | 37 ++--------------------- src/libvirt_private.syms | 3 ++ src/qemu/qemu_domain.c | 19 +----------- src/util/util.c | 4 ++- src/util/virfile.c | 72 ++++++++++++++++++++++++++++++++++++++++++++++ src/util/virfile.h | 8 +++++ src/util/xml.c | 36 +++++++++++++++++++++++ src/util/xml.h | 5 +++ 11 files changed, 141 insertions(+), 177 deletions(-)
I dropped patch 1, replaced virFileSync with fsync in patch 2, moved hunks touching domain_conf.c from patch 3 to patch 4, renamed the structure in patch 3 as suggested by Daniel, and pushed the result. Jirka
participants (3)
-
Daniel P. Berrange -
Eric Blake -
Jiri Denemark