[libvirt] [PATCH] network: Fix upgrade from libvirt older than 1.2.4
Starting from libvirt-1.2.4, network state XML files moved to another directory (see commit b9e95491) and libvirt automatically migrates the network state files to a new location. However, the code used dirent.dt_type which is not supported by all filesystems. Thus, when libvirt is upgraded on a host which uses such filesystem, network state XMLs were not properly moved and running networks disappeared from libvirt. This patch falls back to stat() whenever dirent.dt_type is DT_UNKNOWN to fix this issue. https://bugzilla.redhat.com/show_bug.cgi?id=1167145 Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- src/network/bridge_driver.c | 23 +++++++++++++++++++++-- 1 file changed, 21 insertions(+), 2 deletions(-) diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 6cb421c..7066dfe 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -499,15 +499,34 @@ networkMigrateStateFiles(void) } while ((direrr = virDirRead(dir, &entry, oldStateDir)) > 0) { + if (entry->d_type != DT_UNKNOWN && + entry->d_type != DT_REG) + continue; - if (entry->d_type != DT_REG || - STREQ(entry->d_name, ".") || + if (STREQ(entry->d_name, ".") || STREQ(entry->d_name, "..")) continue; if (virAsprintf(&oldPath, "%s/%s", oldStateDir, entry->d_name) < 0) goto cleanup; + + if (entry->d_type == DT_UNKNOWN) { + struct stat st; + + if (stat(oldPath, &st) < 0) { + virReportSystemError(errno, + _("failed to stat network status file '%s'"), + oldPath); + goto cleanup; + } + + if (!S_ISREG(st.st_mode)) { + VIR_FREE(oldPath); + continue; + } + } + if (virFileReadAll(oldPath, 1024*1024, &contents) < 0) goto cleanup; -- 2.1.3
On 11/26/2014 09:07 AM, Jiri Denemark wrote:
Starting from libvirt-1.2.4, network state XML files moved to another directory (see commit b9e95491) and libvirt automatically migrates the network state files to a new location. However, the code used dirent.dt_type which is not supported by all filesystems. Thus, when libvirt is upgraded on a host which uses such filesystem, network state XMLs were not properly moved and running networks disappeared from libvirt.
This patch falls back to stat() whenever dirent.dt_type is DT_UNKNOWN to
s/dt_type/d_type/
fix this issue.
https://bugzilla.redhat.com/show_bug.cgi?id=1167145 Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- src/network/bridge_driver.c | 23 +++++++++++++++++++++-- 1 file changed, 21 insertions(+), 2 deletions(-)
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 6cb421c..7066dfe 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -499,15 +499,34 @@ networkMigrateStateFiles(void) }
while ((direrr = virDirRead(dir, &entry, oldStateDir)) > 0) { + if (entry->d_type != DT_UNKNOWN && + entry->d_type != DT_REG)
Even the existence of a d_type member is an extension that is not guaranteed to compile. If someone complains, we may have to introduce a helper macro (gnulib fts.c uses a macro D_TYPE(d) that resolves to (d)->d_type where supported, and to DT_UNKNOWN elsewhere; although that particular file is not LGPLv2 so we don't use it in libvirt). But as your patch isn't changing whether something would compile, you don't need to make that change now.
+ continue;
- if (entry->d_type != DT_REG || - STREQ(entry->d_name, ".") || + if (STREQ(entry->d_name, ".") || STREQ(entry->d_name, "..")) continue;
if (virAsprintf(&oldPath, "%s/%s", oldStateDir, entry->d_name) < 0) goto cleanup; + + if (entry->d_type == DT_UNKNOWN) { + struct stat st; + + if (stat(oldPath, &st) < 0) {
Shouldn't this be lstat() instead of stat(), so we aren't chasing symlinks? ACK with that answered. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On Wed, Nov 26, 2014 at 09:16:59 -0700, Eric Blake wrote:
On 11/26/2014 09:07 AM, Jiri Denemark wrote:
Starting from libvirt-1.2.4, network state XML files moved to another directory (see commit b9e95491) and libvirt automatically migrates the network state files to a new location. However, the code used dirent.dt_type which is not supported by all filesystems. Thus, when libvirt is upgraded on a host which uses such filesystem, network state XMLs were not properly moved and running networks disappeared from libvirt.
This patch falls back to stat() whenever dirent.dt_type is DT_UNKNOWN to
s/dt_type/d_type/
fix this issue.
https://bugzilla.redhat.com/show_bug.cgi?id=1167145 Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- src/network/bridge_driver.c | 23 +++++++++++++++++++++-- 1 file changed, 21 insertions(+), 2 deletions(-)
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 6cb421c..7066dfe 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -499,15 +499,34 @@ networkMigrateStateFiles(void) }
while ((direrr = virDirRead(dir, &entry, oldStateDir)) > 0) { + if (entry->d_type != DT_UNKNOWN && + entry->d_type != DT_REG)
Even the existence of a d_type member is an extension that is not guaranteed to compile. If someone complains, we may have to introduce a helper macro (gnulib fts.c uses a macro D_TYPE(d) that resolves to (d)->d_type where supported, and to DT_UNKNOWN elsewhere; although that particular file is not LGPLv2 so we don't use it in libvirt). But as your patch isn't changing whether something would compile, you don't need to make that change now.
Yeah, I was even thinking about making virDirRead wrapper a bit smarter and return our own structure which would always contain d_type. And if d_type was requested by an additional parameter, it would automatically fallback to lstat and set d_type accordingly so that callers do not have to care about this. However, since this is the only caller which cares about it (except for cgroups which work only on Linux anyway), I went with this simpler solution.
+ continue;
- if (entry->d_type != DT_REG || - STREQ(entry->d_name, ".") || + if (STREQ(entry->d_name, ".") || STREQ(entry->d_name, "..")) continue;
if (virAsprintf(&oldPath, "%s/%s", oldStateDir, entry->d_name) < 0) goto cleanup; + + if (entry->d_type == DT_UNKNOWN) { + struct stat st; + + if (stat(oldPath, &st) < 0) {
Shouldn't this be lstat() instead of stat(), so we aren't chasing symlinks?
Indeed.
ACK with that answered.
Thanks, I fixed the issues and pushed this patch. Jirka
participants (2)
-
Eric Blake -
Jiri Denemark