12:26 p.m.
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
12:26 p.m.
New subject: [libvirt] [PATCH 1/4] Introduce virFileSync
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
4:19 a.m.
New subject: [libvirt] [PATCH 1/4] Introduce virFileSync
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 :|
2:42 p.m.
New subject: [libvirt] [PATCH 1/4] Introduce virFileSync
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
12:26 p.m.
New subject: [libvirt] [PATCH 2/4] Introduce virFileRewrite for safe file rewrite
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
4:21 a.m.
New subject: [libvirt] [PATCH 2/4] Introduce virFileRewrite for safe file rewrite
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 :|
5:48 p.m.
New subject: [libvirt] [PATCH 2/4] Introduce virFileRewrite for safe file rewrite
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(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
4:16 p.m.
New subject: [libvirt] [PATCH 2/4] Introduce virFileRewrite for safe file rewrite
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
11:17 a.m.
New subject: [libvirt] [PATCH 2/4] Introduce virFileRewrite for safe file rewrite
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(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
12:26 p.m.
New subject: [libvirt] [PATCH 3/4] Introduce virXMLSaveFile as a wrapper for virFileRewrite
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
4:24 a.m.
New subject: [libvirt] [PATCH 3/4] Introduce virXMLSaveFile as a wrapper for virFileRewrite
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 :|
5:51 p.m.
New subject: [libvirt] [PATCH 3/4] Introduce virXMLSaveFile as a wrapper for virFileRewrite
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(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
3 p.m.
New subject: [libvirt] [PATCH 3/4] Introduce virXMLSaveFile as a wrapper for virFileRewrite
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
12:26 p.m.
New subject: [libvirt] [PATCH 4/4] Use virXMLSaveFile when writing XML config
---
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
4:24 a.m.
New subject: [libvirt] [PATCH 4/4] Use virXMLSaveFile when writing XML config
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 :|
1:34 p.m.
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
4777
days inactive
4785
days old
15 comments
3 participants
participants (3)
-
Daniel P. Berrange -
Eric Blake -
Jiri Denemark