[libvirt] [PATCH] storage_backend_fs.c: do not ignore probe failure
When virStorageBackendProbeTarget fails, it returns -1 or -2. The two uses below obviously intended to handle those cases differently, but due to a wrong comparison, they always treated a "real" (e.g., open) failure (-1) just like an ignorable "wrong file type" failure (-2). FYI, coverity reported that the "goto cleanup" statement was unreachable, which was true in each case, but hardly the real problem.
From c9ba79daf124afbab351741192bbaf7110a199dd Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering@redhat.com> Date: Mon, 18 Jan 2010 10:34:53 +0100 Subject: [PATCH] storage_backend_fs.c: do not ignore probe failure
* src/storage/storage_backend_fs.c (virStorageBackendFileSystemRefresh): Correct two error-handling bugs. The documented intent is to ignore non-regular files, yet due to a comparison "< 0" (rather than "!= 0") all errors were handled that way. (vboxStorageVolDelete): Likewise. (vboxDomainDefineXML): Likewise. --- src/storage/storage_backend_fs.c | 6 +++--- 1 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index 4fe40b3..fa72f9c 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -1,7 +1,7 @@ /* * storage_backend_fs.c: storage backend for FS and directory handling * - * Copyright (C) 2007-2009 Red Hat, Inc. + * Copyright (C) 2007-2010 Red Hat, Inc. * Copyright (C) 2007-2008 Daniel P. Berrange * * This library is free software; you can redistribute it and/or @@ -562,7 +562,7 @@ virStorageBackendFileSystemRefresh(virConnectPtr conn, &backingStore, &vol->allocation, &vol->capacity, - &vol->target.encryption) < 0)) { + &vol->target.encryption) != 0)) { if (ret == -1) goto cleanup; else { @@ -603,7 +603,7 @@ virStorageBackendFileSystemRefresh(virConnectPtr conn, if ((ret = virStorageBackendProbeTarget(conn, &vol->backingStore, NULL, NULL, NULL, - NULL)) < 0) { + NULL)) != 0) { if (ret == -1) goto cleanup; else { -- 1.6.6.638.g2bc54
On Mon, Jan 18, 2010 at 10:40:26AM +0100, Jim Meyering wrote:
When virStorageBackendProbeTarget fails, it returns -1 or -2. The two uses below obviously intended to handle those cases differently, but due to a wrong comparison, they always treated a "real" (e.g., open) failure (-1) just like an ignorable "wrong file type" failure (-2).
FYI, coverity reported that the "goto cleanup" statement was unreachable, which was true in each case, but hardly the real problem.
@@ -562,7 +562,7 @@ virStorageBackendFileSystemRefresh(virConnectPtr conn, &backingStore, &vol->allocation, &vol->capacity, - &vol->target.encryption) < 0)) { + &vol->target.encryption) != 0)) { if (ret == -1) goto cleanup; else { @@ -603,7 +603,7 @@ virStorageBackendFileSystemRefresh(virConnectPtr conn, if ((ret = virStorageBackendProbeTarget(conn, &vol->backingStore, NULL, NULL, NULL, - NULL)) < 0) { + NULL)) != 0) { if (ret == -1) goto cleanup; else {
I don't understand how this is making any difference. The method virStorageBackendProbeTarget can return 0, -1 or -2, so virStorageBackendProbeTarget < 0 catches the -1 and -2 cases virStorageBackendProbeTarget != 0 catches the -1 and -2 cases I only see this change making a difference if one of the error return values is a positive integer greater than 0, but we don't have that here. What am I missing ? Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
Daniel P. Berrange wrote:
On Mon, Jan 18, 2010 at 10:40:26AM +0100, Jim Meyering wrote:
When virStorageBackendProbeTarget fails, it returns -1 or -2. The two uses below obviously intended to handle those cases differently, but due to a wrong comparison, they always treated a "real" (e.g., open) failure (-1) just like an ignorable "wrong file type" failure (-2).
FYI, coverity reported that the "goto cleanup" statement was unreachable, which was true in each case, but hardly the real problem. ...
I don't understand how this is making any difference. The method virStorageBackendProbeTarget can return 0, -1 or -2, so
virStorageBackendProbeTarget < 0 catches the -1 and -2 cases virStorageBackendProbeTarget != 0 catches the -1 and -2 cases
I only see this change making a difference if one of the error return values is a positive integer greater than 0, but we don't have that here. What am I missing ?
Good catch. I changed the wrong thing. Real problem: misplaced ")". Here's the correct patch:
From 8834728541e38c6843c6941457596bba18ca4ae3 Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering@redhat.com> Date: Mon, 18 Jan 2010 10:34:53 +0100 Subject: [PATCH] storage_backend_fs.c: do not ignore probe failure
* src/storage/storage_backend_fs.c (virStorageBackendFileSystemRefresh): Correct parentheses. The documented intent is to ignore non-regular files, yet due to a parenthesization error all errors were handled that way. --- src/storage/storage_backend_fs.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index 4fe40b3..b03e4e9 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -1,7 +1,7 @@ /* * storage_backend_fs.c: storage backend for FS and directory handling * - * Copyright (C) 2007-2009 Red Hat, Inc. + * Copyright (C) 2007-2010 Red Hat, Inc. * Copyright (C) 2007-2008 Daniel P. Berrange * * This library is free software; you can redistribute it and/or @@ -562,7 +562,7 @@ virStorageBackendFileSystemRefresh(virConnectPtr conn, &backingStore, &vol->allocation, &vol->capacity, - &vol->target.encryption) < 0)) { + &vol->target.encryption)) < 0) { if (ret == -1) goto cleanup; else { -- 1.6.6.638.g2bc54
On Mon, Jan 18, 2010 at 01:37:34PM +0100, Jim Meyering wrote:
Daniel P. Berrange wrote:
On Mon, Jan 18, 2010 at 10:40:26AM +0100, Jim Meyering wrote:
When virStorageBackendProbeTarget fails, it returns -1 or -2. The two uses below obviously intended to handle those cases differently, but due to a wrong comparison, they always treated a "real" (e.g., open) failure (-1) just like an ignorable "wrong file type" failure (-2).
FYI, coverity reported that the "goto cleanup" statement was unreachable, which was true in each case, but hardly the real problem. ...
I don't understand how this is making any difference. The method virStorageBackendProbeTarget can return 0, -1 or -2, so
virStorageBackendProbeTarget < 0 catches the -1 and -2 cases virStorageBackendProbeTarget != 0 catches the -1 and -2 cases
I only see this change making a difference if one of the error return values is a positive integer greater than 0, but we don't have that here. What am I missing ?
Good catch. I changed the wrong thing. Real problem: misplaced ")". Here's the correct patch:
From 8834728541e38c6843c6941457596bba18ca4ae3 Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering@redhat.com> Date: Mon, 18 Jan 2010 10:34:53 +0100 Subject: [PATCH] storage_backend_fs.c: do not ignore probe failure
* src/storage/storage_backend_fs.c (virStorageBackendFileSystemRefresh): Correct parentheses. The documented intent is to ignore non-regular files, yet due to a parenthesization error all errors were handled that way. --- src/storage/storage_backend_fs.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index 4fe40b3..b03e4e9 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -1,7 +1,7 @@ /* * storage_backend_fs.c: storage backend for FS and directory handling * - * Copyright (C) 2007-2009 Red Hat, Inc. + * Copyright (C) 2007-2010 Red Hat, Inc. * Copyright (C) 2007-2008 Daniel P. Berrange * * This library is free software; you can redistribute it and/or @@ -562,7 +562,7 @@ virStorageBackendFileSystemRefresh(virConnectPtr conn, &backingStore, &vol->allocation, &vol->capacity, - &vol->target.encryption) < 0)) { + &vol->target.encryption)) < 0) { if (ret == -1) goto cleanup; else {
ACK, makes sense now :-) Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
participants (2)
-
Daniel P. Berrange -
Jim Meyering