Re: [libvirt] [PATCH] network: Fix upgrade from libvirt older than 1.2.4
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(a)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