2 a.m.
We were previously checking for a return < 0 from virFileOperation(),
but that function returns a standard errno, which is 0 on success, or
some small positive number on failure. The result was that we wouldn't
report failures to create storage volume files; instead they would
appear to be created, but then would vanish as soon as a pool-refresh
was done (or cause some later error as soon as someone tried to access
the volume).
The other uses of virFileOperation() were already properly checking
for != 0.
---
src/storage/storage_backend.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c
index aba8937..9792eed 100644
--- a/src/storage/storage_backend.c
+++ b/src/storage/storage_backend.c
@@ -362,7 +362,7 @@ virStorageBackendCreateRaw(virConnectPtr conn ATTRIBUTE_UNUSED,
createRawFileOpHook, &hdata,
VIR_FILE_OP_FORCE_PERMS |
(pool->def->type == VIR_STORAGE_POOL_NETFS
- ? VIR_FILE_OP_AS_UID : 0))) < 0) {
+ ? VIR_FILE_OP_AS_UID : 0))) != 0) {
virReportSystemError(createstat,
_("cannot create path '%s'"),
vol->target.path);
--
1.7.1.1
4:07 a.m.
On Tue, Jul 13, 2010 at 03:00:33AM -0400, Laine Stump wrote:
We were previously checking for a return < 0 from
virFileOperation(),
but that function returns a standard errno, which is 0 on success, or
some small positive number on failure. The result was that we wouldn't
report failures to create storage volume files; instead they would
appear to be created, but then would vanish as soon as a pool-refresh
was done (or cause some later error as soon as someone tried to access
the volume).
The other uses of virFileOperation() were already properly checking
for != 0.
---
src/storage/storage_backend.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c
index aba8937..9792eed 100644
--- a/src/storage/storage_backend.c
+++ b/src/storage/storage_backend.c
@@ -362,7 +362,7 @@ virStorageBackendCreateRaw(virConnectPtr conn ATTRIBUTE_UNUSED,
createRawFileOpHook, &hdata,
VIR_FILE_OP_FORCE_PERMS |
(pool->def->type == VIR_STORAGE_POOL_NETFS
- ? VIR_FILE_OP_AS_UID : 0))) < 0) {
+ ? VIR_FILE_OP_AS_UID : 0))) != 0) {
virReportSystemError(createstat,
_("cannot create path '%s'"),
vol->target.path);
Ah, good caatch, ACK !
Daniel
--
Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/
daniel(a)veillard.com | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
4:47 a.m.
On Tue, Jul 13, 2010 at 03:00:33AM -0400, Laine Stump wrote:
We were previously checking for a return < 0 from
virFileOperation(),
but that function returns a standard errno, which is 0 on success, or
some small positive number on failure. The result was that we wouldn't
report failures to create storage volume files; instead they would
appear to be created, but then would vanish as soon as a pool-refresh
was done (or cause some later error as soon as someone tried to access
the volume).
Urgh, functions that return positive numbers for errors are just
asking for trouble. If you see an 'int' returning function everyone
expects it to return < 0 on error, even if you document otherwise.
The other uses of virFileOperation() were already properly checking
for != 0.
---
src/storage/storage_backend.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c
index aba8937..9792eed 100644
--- a/src/storage/storage_backend.c
+++ b/src/storage/storage_backend.c
@@ -362,7 +362,7 @@ virStorageBackendCreateRaw(virConnectPtr conn ATTRIBUTE_UNUSED,
createRawFileOpHook, &hdata,
VIR_FILE_OP_FORCE_PERMS |
(pool->def->type == VIR_STORAGE_POOL_NETFS
- ? VIR_FILE_OP_AS_UID : 0))) < 0) {
+ ? VIR_FILE_OP_AS_UID : 0))) != 0) {
virReportSystemError(createstat,
_("cannot create path '%s'"),
vol->target.path);
I'd rather we changed virFileOperation to return '-errno' so all error
return codes are negative, and then fixup other callers
Daniel
--
|: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
8:21 p.m.
New subject: [libvirt] [PATCHv2 0/4] Fix return value of several functions
These patches started out as a more "correct" fix for the patch
https://www.redhat.com/archives/libvir-list/2010-July/msg00257.html
(and they supercede that patch).
Daniel Berrange suggested that it would be better to switch the
function virFileOperation over to use the preferred convention of
returning -errno on failure, rather than errno. While I was doing
that, I also changed virDirCreate for consistency.
During this process, I found one bug (in patch 4/4) and made a slight
improvement to another function's return so that virFileOperation's
return would always be -errno (rather than sometimes being a simple
-1). (patch 3/4)
8:21 p.m.
New subject: [libvirt] [PATCHv2 1/4] Change virFileOperation to return -errno (ie < 0) on error.
virFileOperation previously returned 0 on success, or the value of
errno on failure. Although there are other functions in libvirt that
use this convention, the preferred (and more common) convention is to
return 0 on success and -errno (or simply -1 in some cases) on
failure. This way the check for failure is always (ret < 0).
* src/util/util.c - change virFileOperation and virFileOperationNoFork to
return -errno on failure.
* src/storage/storage_backend.c, src/qemu/qemu_driver.c
- change the hook functions passed to virFileOperation to return
-errno on failure.
---
src/qemu/qemu_driver.c | 19 ++++++++++---------
src/storage/storage_backend.c | 17 ++++++++---------
src/util/util.c | 37 ++++++++++++++++++++-----------------
3 files changed, 38 insertions(+), 35 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 2eb254e..4818be3 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -4946,12 +4946,13 @@ struct fileOpHookData {
struct qemud_save_header *header;
};
+/* return -errno on failure, or 0 on success */
static int qemudDomainSaveFileOpHook(int fd, void *data) {
struct fileOpHookData *hdata = data;
int ret = 0;
if (safewrite(fd, hdata->header, sizeof(*hdata->header)) !=
sizeof(*hdata->header)) {
- ret = errno;
+ ret = -errno;
qemuReportError(VIR_ERR_OPERATION_FAILED,
_("failed to write header to domain save file
'%s'"),
hdata->path);
@@ -4959,7 +4960,7 @@ static int qemudDomainSaveFileOpHook(int fd, void *data) {
}
if (safewrite(fd, hdata->xml, hdata->header->xml_len) !=
hdata->header->xml_len) {
- ret = errno;
+ ret = -errno;
qemuReportError(VIR_ERR_OPERATION_FAILED,
_("failed to write xml to '%s'"),
hdata->path);
goto endjob;
@@ -5095,7 +5096,7 @@ static int qemudDomainSaveFlag(virDomainPtr dom, const char *path,
virReportSystemError(errno, _("unable to open %s"), path);
goto endjob;
}
- if (qemudDomainSaveFileOpHook(fd, &hdata) != 0) {
+ if (qemudDomainSaveFileOpHook(fd, &hdata) < 0) {
close(fd);
goto endjob;
}
@@ -5108,7 +5109,7 @@ static int qemudDomainSaveFlag(virDomainPtr dom, const char *path,
S_IRUSR|S_IWUSR,
getuid(), getgid(),
qemudDomainSaveFileOpHook, &hdata,
- 0)) != 0) {
+ 0)) < 0) {
/* If we failed as root, and the error was permission-denied
(EACCES), assume it's on a network-connected share where
root access is restricted (eg, root-squashed NFS). If the
@@ -5116,9 +5117,9 @@ static int qemudDomainSaveFlag(virDomainPtr dom, const char *path,
bypass security driver shenanigans, and retry the operation
after doing setuid to qemu user */
- if ((rc != EACCES) ||
+ if ((rc != -EACCES) ||
driver->user == getuid()) {
- virReportSystemError(rc, _("Failed to create domain save file
'%s'"),
+ virReportSystemError(-rc, _("Failed to create domain save file
'%s'"),
path);
goto endjob;
}
@@ -5142,7 +5143,7 @@ static int qemudDomainSaveFlag(virDomainPtr dom, const char *path,
case 0:
default:
/* local file - log the error returned by virFileOperation */
- virReportSystemError(rc,
+ virReportSystemError(-rc,
_("Failed to create domain save file
'%s'"),
path);
goto endjob;
@@ -5156,8 +5157,8 @@ static int qemudDomainSaveFlag(virDomainPtr dom, const char *path,
S_IRUSR|S_IWUSR|S_IRGRP|S_IWGRP,
driver->user, driver->group,
qemudDomainSaveFileOpHook, &hdata,
- VIR_FILE_OP_AS_UID)) != 0) {
- virReportSystemError(rc, _("Error from child process creating
'%s'"),
+ VIR_FILE_OP_AS_UID)) < 0) {
+ virReportSystemError(-rc, _("Error from child process creating
'%s'"),
path);
goto endjob;
}
diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c
index 730ca7b..23adea7 100644
--- a/src/storage/storage_backend.c
+++ b/src/storage/storage_backend.c
@@ -276,7 +276,7 @@ static int createRawFileOpHook(int fd, void *data) {
/* Seek to the final size, so the capacity is available upfront
* for progress reporting */
if (ftruncate(fd, hdata->vol->capacity) < 0) {
- ret = errno;
+ ret = -errno;
virReportSystemError(errno,
_("cannot extend file '%s'"),
hdata->vol->target.path);
@@ -286,10 +286,9 @@ static int createRawFileOpHook(int fd, void *data) {
remain = hdata->vol->allocation;
if (hdata->inputvol) {
- int res = virStorageBackendCopyToFD(hdata->vol, hdata->inputvol,
- fd, &remain, 1);
- if (res < 0) {
- ret = -res;
+ ret = virStorageBackendCopyToFD(hdata->vol, hdata->inputvol,
+ fd, &remain, 1);
+ if (ret < 0) {
goto cleanup;
}
}
@@ -308,7 +307,7 @@ static int createRawFileOpHook(int fd, void *data) {
bytes = remain;
if (safezero(fd, 0, hdata->vol->allocation - remain,
bytes) != 0) {
- ret = errno;
+ ret = -errno;
virReportSystemError(errno, _("cannot fill file
'%s'"),
hdata->vol->target.path);
goto cleanup;
@@ -317,7 +316,7 @@ static int createRawFileOpHook(int fd, void *data) {
}
} else { /* No progress bars to be shown */
if (safezero(fd, 0, 0, remain) != 0) {
- ret = errno;
+ ret = -errno;
virReportSystemError(errno, _("cannot fill file
'%s'"),
hdata->vol->target.path);
goto cleanup;
@@ -327,7 +326,7 @@ static int createRawFileOpHook(int fd, void *data) {
}
if (fsync(fd) < 0) {
- ret = errno;
+ ret = -errno;
virReportSystemError(errno, _("cannot sync data to file
'%s'"),
hdata->vol->target.path);
goto cleanup;
@@ -365,7 +364,7 @@ virStorageBackendCreateRaw(virConnectPtr conn ATTRIBUTE_UNUSED,
VIR_FILE_OP_FORCE_PERMS |
(pool->def->type == VIR_STORAGE_POOL_NETFS
? VIR_FILE_OP_AS_UID : 0))) < 0) {
- virReportSystemError(createstat,
+ virReportSystemError(-createstat,
_("cannot create path '%s'"),
vol->target.path);
goto cleanup;
diff --git a/src/util/util.c b/src/util/util.c
index a04c515..ee5dd5e 100644
--- a/src/util/util.c
+++ b/src/util/util.c
@@ -1267,6 +1267,7 @@ int virFileExists(const char *path)
}
# ifndef WIN32
+/* return -errno on failure, or 0 on success */
static int virFileOperationNoFork(const char *path, int openflags, mode_t mode,
uid_t uid, gid_t gid,
virFileOperationHook hook, void *hookdata,
@@ -1276,26 +1277,26 @@ static int virFileOperationNoFork(const char *path, int openflags,
mode_t mode,
struct stat st;
if ((fd = open(path, openflags, mode)) < 0) {
- ret = errno;
+ ret = -errno;
virReportSystemError(errno, _("failed to create file '%s'"),
path);
goto error;
}
if (fstat(fd, &st) == -1) {
- ret = errno;
+ ret = -errno;
virReportSystemError(errno, _("stat of '%s' failed"), path);
goto error;
}
if (((st.st_uid != uid) || (st.st_gid != gid))
&& (fchown(fd, uid, gid) < 0)) {
- ret = errno;
+ ret = -errno;
virReportSystemError(errno, _("cannot chown '%s' to (%u,
%u)"),
path, (unsigned int) uid, (unsigned int) gid);
goto error;
}
if ((flags & VIR_FILE_OP_FORCE_PERMS)
&& (fchmod(fd, mode) < 0)) {
- ret = errno;
+ ret = -errno;
virReportSystemError(errno,
_("cannot set mode of '%s' to %04o"),
path, mode);
@@ -1305,7 +1306,7 @@ static int virFileOperationNoFork(const char *path, int openflags,
mode_t mode,
goto error;
}
if (close(fd) < 0) {
- ret = errno;
+ ret = -errno;
virReportSystemError(errno, _("failed to close new file
'%s'"),
path);
fd = -1;
@@ -1356,6 +1357,7 @@ error:
return ret;
}
+/* return -errno on failure, or 0 on success */
int virFileOperation(const char *path, int openflags, mode_t mode,
uid_t uid, gid_t gid,
virFileOperationHook hook, void *hookdata,
@@ -1380,7 +1382,7 @@ int virFileOperation(const char *path, int openflags, mode_t mode,
int forkRet = virFork(&pid);
if (pid < 0) {
- ret = errno;
+ ret = -errno;
return ret;
}
@@ -1389,14 +1391,14 @@ int virFileOperation(const char *path, int openflags, mode_t
mode,
while ((waitret = waitpid(pid, &status, 0) == -1)
&& (errno == EINTR));
if (waitret == -1) {
- ret = errno;
+ ret = -errno;
virReportSystemError(errno,
_("failed to wait for child creating
'%s'"),
path);
goto parenterror;
}
- ret = WEXITSTATUS(status);
- if (!WIFEXITED(status) || (ret == EACCES)) {
+ ret = -WEXITSTATUS(status);
+ if (!WIFEXITED(status) || (ret == -EACCES)) {
/* fall back to the simpler method, which works better in
* some cases */
return virFileOperationNoFork(path, openflags, mode, uid, gid,
@@ -1417,22 +1419,22 @@ parenterror:
/* set desired uid/gid, then attempt to create the file */
if ((gid != 0) && (setgid(gid) != 0)) {
- ret = errno;
+ ret = -errno;
virReportSystemError(errno,
_("cannot set gid %u creating '%s'"),
(unsigned int) gid, path);
goto childerror;
}
if ((uid != 0) && (setuid(uid) != 0)) {
- ret = errno;
+ ret = -errno;
virReportSystemError(errno,
_("cannot set uid %u creating '%s'"),
(unsigned int) uid, path);
goto childerror;
}
if ((fd = open(path, openflags, mode)) < 0) {
- ret = errno;
- if (ret != EACCES) {
+ ret = -errno;
+ if (ret != -EACCES) {
/* in case of EACCES, the parent will retry */
virReportSystemError(errno,
_("child failed to create file
'%s'"),
@@ -1441,20 +1443,20 @@ parenterror:
goto childerror;
}
if (fstat(fd, &st) == -1) {
- ret = errno;
+ ret = -errno;
virReportSystemError(errno, _("stat of '%s' failed"), path);
goto childerror;
}
if ((st.st_gid != gid)
&& (fchown(fd, -1, gid) < 0)) {
- ret = errno;
+ ret = -errno;
virReportSystemError(errno, _("cannot chown '%s' to (%u,
%u)"),
path, (unsigned int) uid, (unsigned int) gid);
goto childerror;
}
if ((flags & VIR_FILE_OP_FORCE_PERMS)
&& (fchmod(fd, mode) < 0)) {
- ret = errno;
+ ret = -errno;
virReportSystemError(errno,
_("cannot set mode of '%s' to %04o"),
path, mode);
@@ -1464,7 +1466,7 @@ parenterror:
goto childerror;
}
if (close(fd) < 0) {
- ret = errno;
+ ret = -errno;
virReportSystemError(errno, _("child failed to close new file
'%s'"),
path);
goto childerror;
@@ -1576,6 +1578,7 @@ childerror:
# else /* WIN32 */
+/* return -errno on failure, or 0 on success */
int virFileOperation(const char *path ATTRIBUTE_UNUSED,
int openflags ATTRIBUTE_UNUSED,
mode_t mode ATTRIBUTE_UNUSED,
--
1.7.1.1
10:11 a.m.
New subject: [libvirt] [PATCHv2 1/4] Change virFileOperation to return -errno (ie < 0) on error.
On Mon, Jul 19, 2010 at 09:21:52PM -0400, Laine Stump wrote:
virFileOperation previously returned 0 on success, or the value of
errno on failure. Although there are other functions in libvirt that
use this convention, the preferred (and more common) convention is to
return 0 on success and -errno (or simply -1 in some cases) on
failure. This way the check for failure is always (ret < 0).
* src/util/util.c - change virFileOperation and virFileOperationNoFork to
return -errno on failure.
* src/storage/storage_backend.c, src/qemu/qemu_driver.c
- change the hook functions passed to virFileOperation to return
-errno on failure.
ACK,
Daniel
--
Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/
daniel(a)veillard.com | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
1:33 p.m.
New subject: [libvirt] [PATCHv2 1/4] Change virFileOperation to return -errno (ie < 0) on error.
On 07/21/2010 11:11 AM, Daniel Veillard wrote:
On Mon, Jul 19, 2010 at 09:21:52PM -0400, Laine Stump wrote:
> virFileOperation previously returned 0 on success, or the value of
> errno on failure. Although there are other functions in libvirt that
> use this convention, the preferred (and more common) convention is to
> return 0 on success and -errno (or simply -1 in some cases) on
> failure. This way the check for failure is always (ret< 0).
>
> * src/util/util.c - change virFileOperation and virFileOperationNoFork to
> return -errno on failure.
>
> * src/storage/storage_backend.c, src/qemu/qemu_driver.c
> - change the hook functions passed to virFileOperation to return
> -errno on failure.
ACK,
Pushed. Thanks!
8:21 p.m.
New subject: [libvirt] [PATCHv2 2/4] Make virStorageBackendCopyToFD return -errno.
Previously virStorageBackendCopyToFD would simply return -1 on
error. This made the error return from one of its callers inconsistent
(createRawFileOpHook is supposed to return -errno, but if
virStorageBackendCopyToFD failed, createRawFileOpHook would just
return -1). Since there is a useful errno in every case of error
return from virStorageBackendCopyToFD, and since the other uses of
that function ignore the return code (beyond simply checking to see if
it is < 0), this is a safe change.
---
src/storage/storage_backend.c | 9 +++++++--
1 files changed, 7 insertions(+), 2 deletions(-)
diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c
index 23adea7..5b61bba 100644
--- a/src/storage/storage_backend.c
+++ b/src/storage/storage_backend.c
@@ -116,13 +116,14 @@ virStorageBackendCopyToFD(virStorageVolDefPtr vol,
{
int inputfd = -1;
int amtread = -1;
- int ret = -1;
+ int ret = 0;
unsigned long long remain;
size_t bytes = 1024 * 1024;
char zerobuf[512];
char *buf = NULL;
if ((inputfd = open(inputvol->target.path, O_RDONLY)) < 0) {
+ ret = -errno;
virReportSystemError(errno,
_("could not open input path '%s'"),
inputvol->target.path);
@@ -132,6 +133,7 @@ virStorageBackendCopyToFD(virStorageVolDefPtr vol,
bzero(&zerobuf, sizeof(zerobuf));
if (VIR_ALLOC_N(buf, bytes) < 0) {
+ ret = -errno;
virReportOOMError();
goto cleanup;
}
@@ -145,6 +147,7 @@ virStorageBackendCopyToFD(virStorageVolDefPtr vol,
bytes = remain;
if ((amtread = saferead(inputfd, buf, bytes)) < 0) {
+ ret = -errno;
virReportSystemError(errno,
_("failed reading from file '%s'"),
inputvol->target.path);
@@ -161,12 +164,14 @@ virStorageBackendCopyToFD(virStorageVolDefPtr vol,
if (is_dest_file && memcmp(buf+offset, zerobuf, interval) == 0) {
if (lseek(fd, interval, SEEK_CUR) < 0) {
+ ret = -errno;
virReportSystemError(errno,
_("cannot extend file '%s'"),
vol->target.path);
goto cleanup;
}
} else if (safewrite(fd, buf+offset, interval) < 0) {
+ ret = -errno;
virReportSystemError(errno,
_("failed writing to file '%s'"),
vol->target.path);
@@ -177,6 +182,7 @@ virStorageBackendCopyToFD(virStorageVolDefPtr vol,
}
if (inputfd != -1 && close(inputfd) < 0) {
+ ret = -errno;
virReportSystemError(errno,
_("cannot close file '%s'"),
inputvol->target.path);
@@ -185,7 +191,6 @@ virStorageBackendCopyToFD(virStorageVolDefPtr vol,
inputfd = -1;
*total -= remain;
- ret = 0;
cleanup:
if (inputfd != -1)
--
1.7.1.1
10:12 a.m.
New subject: [libvirt] [PATCHv2 2/4] Make virStorageBackendCopyToFD return -errno.
On Mon, Jul 19, 2010 at 09:21:53PM -0400, Laine Stump wrote:
Previously virStorageBackendCopyToFD would simply return -1 on
error. This made the error return from one of its callers inconsistent
(createRawFileOpHook is supposed to return -errno, but if
virStorageBackendCopyToFD failed, createRawFileOpHook would just
return -1). Since there is a useful errno in every case of error
return from virStorageBackendCopyToFD, and since the other uses of
that function ignore the return code (beyond simply checking to see if
it is < 0), this is a safe change.
---
src/storage/storage_backend.c | 9 +++++++--
1 files changed, 7 insertions(+), 2 deletions(-)
ACK,
Daniel
--
Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/
daniel(a)veillard.com | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
1:34 p.m.
New subject: [libvirt] [PATCHv2 2/4] Make virStorageBackendCopyToFD return -errno.
On 07/21/2010 11:12 AM, Daniel Veillard wrote:
On Mon, Jul 19, 2010 at 09:21:53PM -0400, Laine Stump wrote:
> Previously virStorageBackendCopyToFD would simply return -1 on
> error. This made the error return from one of its callers inconsistent
> (createRawFileOpHook is supposed to return -errno, but if
> virStorageBackendCopyToFD failed, createRawFileOpHook would just
> return -1). Since there is a useful errno in every case of error
> return from virStorageBackendCopyToFD, and since the other uses of
> that function ignore the return code (beyond simply checking to see if
> it is< 0), this is a safe change.
> ---
> src/storage/storage_backend.c | 9 +++++++--
> 1 files changed, 7 insertions(+), 2 deletions(-)
>
ACK,
Pushed. Thanks!
8:21 p.m.
New subject: [libvirt] [PATCHv2 3/4] Change virDirCreate to return -errno on failure.
virDirCreate also previously returned 0 on success and errno on
failure. This makes it fit the recommended convention of returning 0
on success, -errno (ie a negative number) on failure.
---
src/storage/storage_backend_fs.c | 8 ++++----
src/util/util.c | 32 +++++++++++++++++---------------
2 files changed, 21 insertions(+), 19 deletions(-)
diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c
index ffb0071..0761bd8 100644
--- a/src/storage/storage_backend_fs.c
+++ b/src/storage/storage_backend_fs.c
@@ -567,8 +567,8 @@ virStorageBackendFileSystemBuild(virConnectPtr conn ATTRIBUTE_UNUSED,
VIR_DIR_CREATE_FORCE_PERMS |
VIR_DIR_CREATE_ALLOW_EXIST |
(pool->def->type == VIR_STORAGE_POOL_NETFS
- ? VIR_DIR_CREATE_AS_UID : 0)) != 0)) {
- virReportSystemError(err, _("cannot create path '%s'"),
+ ? VIR_DIR_CREATE_AS_UID : 0)) < 0)) {
+ virReportSystemError(-err, _("cannot create path '%s'"),
pool->def->target.path);
goto error;
}
@@ -793,8 +793,8 @@ static int createFileDir(virConnectPtr conn ATTRIBUTE_UNUSED,
uid, gid,
VIR_DIR_CREATE_FORCE_PERMS |
(pool->def->type == VIR_STORAGE_POOL_NETFS
- ? VIR_DIR_CREATE_AS_UID : 0))) != 0) {
- virReportSystemError(err, _("cannot create path '%s'"),
+ ? VIR_DIR_CREATE_AS_UID : 0))) < 0) {
+ virReportSystemError(-err, _("cannot create path '%s'"),
vol->target.path);
return -1;
}
diff --git a/src/util/util.c b/src/util/util.c
index ee5dd5e..8f2a17e 100644
--- a/src/util/util.c
+++ b/src/util/util.c
@@ -1319,6 +1319,7 @@ error:
return ret;
}
+/* return -errno on failure, or 0 on success */
static int virDirCreateNoFork(const char *path, mode_t mode, uid_t uid, gid_t gid,
unsigned int flags) {
int ret = 0;
@@ -1327,27 +1328,27 @@ static int virDirCreateNoFork(const char *path, mode_t mode, uid_t
uid, gid_t gi
if ((mkdir(path, mode) < 0)
&& !((errno == EEXIST) && (flags &
VIR_DIR_CREATE_ALLOW_EXIST)))
{
- ret = errno;
+ ret = -errno;
virReportSystemError(errno, _("failed to create directory
'%s'"),
path);
goto error;
}
if (stat(path, &st) == -1) {
- ret = errno;
+ ret = -errno;
virReportSystemError(errno, _("stat of '%s' failed"), path);
goto error;
}
if (((st.st_uid != uid) || (st.st_gid != gid))
&& (chown(path, uid, gid) < 0)) {
- ret = errno;
+ ret = -errno;
virReportSystemError(errno, _("cannot chown '%s' to (%u,
%u)"),
path, (unsigned int) uid, (unsigned int) gid);
goto error;
}
if ((flags & VIR_DIR_CREATE_FORCE_PERMS)
&& (chmod(path, mode) < 0)) {
- ret = errno;
+ ret = -errno;
virReportSystemError(errno,
_("cannot set mode of '%s' to %04o"),
path, mode);
@@ -1476,6 +1477,7 @@ childerror:
}
+/* return -errno on failure, or 0 on success */
int virDirCreate(const char *path, mode_t mode,
uid_t uid, gid_t gid, unsigned int flags) {
struct stat st;
@@ -1493,7 +1495,7 @@ int virDirCreate(const char *path, mode_t mode,
int forkRet = virFork(&pid);
if (pid < 0) {
- ret = errno;
+ ret = -errno;
return ret;
}
@@ -1501,19 +1503,19 @@ int virDirCreate(const char *path, mode_t mode,
/* wait for child to complete, and retrieve its exit code */
while ((waitret = waitpid(pid, &status, 0) == -1) && (errno ==
EINTR));
if (waitret == -1) {
- ret = errno;
+ ret = -errno;
virReportSystemError(errno,
_("failed to wait for child creating
'%s'"),
path);
goto parenterror;
}
- ret = WEXITSTATUS(status);
- if (!WIFEXITED(status) || (ret == EACCES)) {
+ ret = -WEXITSTATUS(status);
+ if (!WIFEXITED(status) || (ret == -EACCES)) {
/* fall back to the simpler method, which works better in
* some cases */
return virDirCreateNoFork(path, mode, uid, gid, flags);
}
- if (ret != 0) {
+ if (ret < 0) {
goto parenterror;
}
parenterror:
@@ -1530,20 +1532,20 @@ parenterror:
/* set desired uid/gid, then attempt to create the directory */
if ((gid != 0) && (setgid(gid) != 0)) {
- ret = errno;
+ ret = -errno;
virReportSystemError(errno, _("cannot set gid %u creating
'%s'"),
(unsigned int) gid, path);
goto childerror;
}
if ((uid != 0) && (setuid(uid) != 0)) {
- ret = errno;
+ ret = -errno;
virReportSystemError(errno, _("cannot set uid %u creating
'%s'"),
(unsigned int) uid, path);
goto childerror;
}
if (mkdir(path, mode) < 0) {
- ret = errno;
- if (ret != EACCES) {
+ ret = -errno;
+ if (ret != -EACCES) {
/* in case of EACCES, the parent will retry */
virReportSystemError(errno, _("child failed to create directory
'%s'"),
path);
@@ -1553,13 +1555,13 @@ parenterror:
/* check if group was set properly by creating after
* setgid. If not, try doing it with chown */
if (stat(path, &st) == -1) {
- ret = errno;
+ ret = -errno;
virReportSystemError(errno,
_("stat of '%s' failed"), path);
goto childerror;
}
if ((st.st_gid != gid) && (chown(path, -1, gid) < 0)) {
- ret = errno;
+ ret = -errno;
virReportSystemError(errno,
_("cannot chown '%s' to group %u"),
path, (unsigned int) gid);
--
1.7.1.1
10:17 a.m.
New subject: [libvirt] [PATCHv2 3/4] Change virDirCreate to return -errno on failure.
On Mon, Jul 19, 2010 at 09:21:54PM -0400, Laine Stump wrote:
virDirCreate also previously returned 0 on success and errno on
failure. This makes it fit the recommended convention of returning 0
on success, -errno (ie a negative number) on failure.
---
src/storage/storage_backend_fs.c | 8 ++++----
src/util/util.c | 32 +++++++++++++++++---------------
2 files changed, 21 insertions(+), 19 deletions(-)
diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c
index ffb0071..0761bd8 100644
ACK,
Daniel
--
Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/
daniel(a)veillard.com | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
1:34 p.m.
New subject: [libvirt] [PATCHv2 3/4] Change virDirCreate to return -errno on failure.
On 07/21/2010 11:17 AM, Daniel Veillard wrote:
On Mon, Jul 19, 2010 at 09:21:54PM -0400, Laine Stump wrote:
> virDirCreate also previously returned 0 on success and errno on
> failure. This makes it fit the recommended convention of returning 0
> on success, -errno (ie a negative number) on failure.
> ---
> src/storage/storage_backend_fs.c | 8 ++++----
> src/util/util.c | 32 +++++++++++++++++---------------
> 2 files changed, 21 insertions(+), 19 deletions(-)
>
> diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c
> index ffb0071..0761bd8 100644
ACK,
Pushed. Thanks!
8:21 p.m.
New subject: [libvirt] [PATCHv2 4/4] Remove erroneous setting of return value to errno.
One error exit in virStorageBackendCreateBlockFrom was setting the
return value to errno. The convention for volume build functions is to
return 0 on success or -1 on failure. Not only was it not necessary to
set the return value (it defaults to -1, and is set to 0 when
everything has been successfully completed), in the case that some
caller were checking for < 0 rather than != 0, they would incorrectly
believe that it completed successfully.
---
src/storage/storage_backend.c | 1 -
1 files changed, 0 insertions(+), 1 deletions(-)
diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c
index 5b61bba..d989743 100644
--- a/src/storage/storage_backend.c
+++ b/src/storage/storage_backend.c
@@ -232,7 +232,6 @@ virStorageBackendCreateBlockFrom(virConnectPtr conn ATTRIBUTE_UNUSED,
}
if (fstat(fd, &st) == -1) {
- ret = errno;
virReportSystemError(errno, _("stat of '%s' failed"),
vol->target.path);
goto cleanup;
--
1.7.1.1
10:21 a.m.
New subject: [libvirt] [PATCHv2 4/4] Remove erroneous setting of return value to errno.
On Mon, Jul 19, 2010 at 09:21:55PM -0400, Laine Stump wrote:
One error exit in virStorageBackendCreateBlockFrom was setting the
return value to errno. The convention for volume build functions is to
return 0 on success or -1 on failure. Not only was it not necessary to
set the return value (it defaults to -1, and is set to 0 when
everything has been successfully completed), in the case that some
caller were checking for < 0 rather than != 0, they would incorrectly
believe that it completed successfully.
---
src/storage/storage_backend.c | 1 -
1 files changed, 0 insertions(+), 1 deletions(-)
diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c
index 5b61bba..d989743 100644
--- a/src/storage/storage_backend.c
+++ b/src/storage/storage_backend.c
@@ -232,7 +232,6 @@ virStorageBackendCreateBlockFrom(virConnectPtr conn
ATTRIBUTE_UNUSED,
}
if (fstat(fd, &st) == -1) {
- ret = errno;
virReportSystemError(errno, _("stat of '%s' failed"),
vol->target.path);
goto cleanup;
ACK, but it seems to me ret being initialized to -1, maybe it's better
to be consistent and on all error do a ret = -errno; before
virReportSystemError and the goto cleanup.
Daniel
--
Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/
daniel(a)veillard.com | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
10:37 a.m.
New subject: [libvirt] [PATCHv2 4/4] Remove erroneous setting of return value to errno.
On 07/21/2010 09:21 AM, Daniel Veillard wrote:
ACK, but it seems to me ret being initialized to -1, maybe it's better
to be consistent and on all error do a ret = -errno; before
virReportSystemError and the goto cleanup.
If you have the convention of returning -errno, then you must not return
-1 unless you meant to report EPERM. I agree with Laine's approach of
initializing to 0 in this case, since you don't want to leak an
unintended -1 if that was not the real error.
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
1:28 p.m.
New subject: [libvirt] [PATCHv2 4/4] Remove erroneous setting of return value to errno.
On 07/21/2010 11:37 AM, Eric Blake wrote:
On 07/21/2010 09:21 AM, Daniel Veillard wrote:
> ACK, but it seems to me ret being initialized to -1, maybe it's better
> to be consistent and on all error do a ret = -errno; before
> virReportSystemError and the goto cleanup.
If you have the convention of returning -errno, then you must not return
-1 unless you meant to report EPERM. I agree with Laine's approach of
initializing to 0 in this case, since you don't want to leak an
unintended -1 if that was not the real error.
In the case of this function, the patch isn't intended to make the
function return -errno (like the others in this series), it was just to
make it consistent; this was a bug that I coincidentally noticed while
changing the other functions from "return positive errno on failure" to
"return -errno on failure". I think DV was wondering out loud if we
should change the convention of this function (from "return -1 on
error") as well.
In order to do that, I would have to 1) change all other functions that
are called interchangeably with this function (ie, anything assigned to
create_func in storage_backend_fs.c, assigned to .buildVolFrom, or a
couple other places), and 2) assure that all places any of these are
called are okay with a return that is merely < 0, rather than explicitly
-1. (and we would also need to make sure that there was a reasonable
errno to return in every error case; for example in some functions the
error is caused by a configuration problem, not by some system failure).
Instead of pushing this change right away, I'll go through all those
functions to see if such a change is possible.
4:29 p.m.
New subject: [libvirt] [PATCHv2 4/4] Remove erroneous setting of return value to errno.
On 07/21/2010 02:28 PM, Laine Stump wrote:
On 07/21/2010 11:37 AM, Eric Blake wrote:
> On 07/21/2010 09:21 AM, Daniel Veillard wrote:
>> ACK, but it seems to me ret being initialized to -1, maybe it's
>> better
>> to be consistent and on all error do a ret = -errno; before
>> virReportSystemError and the goto cleanup.
> If you have the convention of returning -errno, then you must not return
> -1 unless you meant to report EPERM. I agree with Laine's approach of
> initializing to 0 in this case, since you don't want to leak an
> unintended -1 if that was not the real error.
>
In the case of this function, the patch isn't intended to make
the
function return
-errno (like the others in this series), it was just to make it
consistent; this was
a bug that I coincidentally noticed while changing the other
functions from
"return positive errno on failure" to "return -errno
on failure". I
think DV was
wondering out loud if we should change the convention of this
function (from
"return -1 on error") as well.
In order to do that, I would have to 1) change all other functions
that are
called interchangeably with this function (ie, anything assigned to
create_func
in storage_backend_fs.c, assigned to .buildVolFrom, or a couple other
places),
and 2) assure that all places any of these are called are okay with a
return
that is merely < 0, rather than explicitly -1. (and we would also
need to make
sure that there was a reasonable errno to return in every error case;
for
example in some functions the error is caused by a configuration
problem, not
by some system failure).
Instead of pushing this change right away, I'll go through all those
functions
to see if such a change is possible.
I looked at this more, and saw that this function is just one of "many"
(probably a couple dozen) functions that are used by the storage driver
and pointed to by members of virStorageBackend tables. All of these
functions consistently return 0 on success and -1 on error. If I were
going to change this one function (virStorageBackendCreateBlockFrom), I
should then change any function that could be set as a .buildFrom member
of virStorageBackend, and if I did that, I should change all functions
that could be any of the members of virStorageBackend. The problem is
that in some cases, there is no appropriate errno to return, and none of
the callers expect that anyway.
So in the end, I've decided to leave this function returning -1 on
failure, and 0 on success (and am pushing the patch as-is, since it
eliminates something that violates that convention).
5238
days inactive
5246
days old
17 comments
4 participants
participants (4)
-
Daniel P. Berrange -
Daniel Veillard -
Eric Blake -
Laine Stump