| Message ID | 1346833574-26193-1-git-send-email-riegamaths@gmail.com |
|---|---|
| State | New |
| Headers | show |
On Wed, Sep 05, 2012 at 04:26:14PM +0800, riegamaths@gmail.com wrote: > From: Dunrong Huang <riegamaths@gmail.com> > > If we failed to create temporary snapshot, the error message did not match > with the error, for example: > > $ TMPDIR=/tmp/bad_path qemu-system-x86_64 -enable-kvm debian.qcow2 -snapshot > qemu-system-x86_64: -enable-kvm: could not open disk image /home/mathslinux/Images/debian.qcow2: No such file or directory > > Indeed, the file which cant be created is /tmp/bad_path/vl.xxxxxx, not > debian.qcow2. so the error message makes users feel confused. > > Signed-off-by: Dunrong Huang <riegamaths@gmail.com> > --- > block.c | 2 ++ > 1 个文件被修改，插入 2 行(+) > > diff --git a/block.c b/block.c > index 470bdcc..c9e5140 100644 > --- a/block.c > +++ b/block.c > @@ -434,6 +434,8 @@ int get_tmp_filename(char *filename, int size) > } > fd = mkstemp(filename); > if (fd < 0 || close(fd)) { > + fprintf(stderr, "Could not create temporary snapshot in %s directory: " > + "%s\n", tmpdir, strerror(errno)); The error message is fine for fd < 0 but not for close(0) != 0. Also, close(2) is allowed to change errno (even on success) so this is not portable and could clobber the errno value. Please split into: if (fd < 0) { ... } if (close(fd) != 0) { ... } Stefan
Am 05.09.2012 11:40, schrieb Stefan Hajnoczi: > On Wed, Sep 05, 2012 at 04:26:14PM +0800, riegamaths@gmail.com wrote: >> From: Dunrong Huang <riegamaths@gmail.com> >> >> If we failed to create temporary snapshot, the error message did not match >> with the error, for example: >> >> $ TMPDIR=/tmp/bad_path qemu-system-x86_64 -enable-kvm debian.qcow2 -snapshot >> qemu-system-x86_64: -enable-kvm: could not open disk image /home/mathslinux/Images/debian.qcow2: No such file or directory >> >> Indeed, the file which cant be created is /tmp/bad_path/vl.xxxxxx, not >> debian.qcow2. so the error message makes users feel confused. >> >> Signed-off-by: Dunrong Huang <riegamaths@gmail.com> >> --- >> block.c | 2 ++ >> 1 个文件被修改，插入 2 行(+) >> >> diff --git a/block.c b/block.c >> index 470bdcc..c9e5140 100644 >> --- a/block.c >> +++ b/block.c >> @@ -434,6 +434,8 @@ int get_tmp_filename(char *filename, int size) >> } >> fd = mkstemp(filename); >> if (fd < 0 || close(fd)) { >> + fprintf(stderr, "Could not create temporary snapshot in %s directory: " >> + "%s\n", tmpdir, strerror(errno)); > > The error message is fine for fd < 0 but not for close(0) != 0. Also, > close(2) is allowed to change errno (even on success) so this is not > portable and could clobber the errno value. I don't think this error message is fine in get_tmp_filename(). This function happens to be used for temporary snapshots, but this is not part of its interface. Today it's also used in vvfat and there the message would only be confusing. Other use cases may be introduced in the future. If you want to introduce an error message, do it in the caller. Kevin
2012/9/5 Kevin Wolf <kwolf@redhat.com>: > Am 05.09.2012 11:40, schrieb Stefan Hajnoczi: >> On Wed, Sep 05, 2012 at 04:26:14PM +0800, riegamaths@gmail.com wrote: >>> From: Dunrong Huang <riegamaths@gmail.com> >>> >>> If we failed to create temporary snapshot, the error message did not match >>> with the error, for example: >>> >>> $ TMPDIR=/tmp/bad_path qemu-system-x86_64 -enable-kvm debian.qcow2 -snapshot >>> qemu-system-x86_64: -enable-kvm: could not open disk image /home/mathslinux/Images/debian.qcow2: No such file or directory >>> >>> Indeed, the file which cant be created is /tmp/bad_path/vl.xxxxxx, not >>> debian.qcow2. so the error message makes users feel confused. >>> >>> Signed-off-by: Dunrong Huang <riegamaths@gmail.com> >>> --- >>> block.c | 2 ++ >>> 1 个文件被修改，插入 2 行(+) >>> >>> diff --git a/block.c b/block.c >>> index 470bdcc..c9e5140 100644 >>> --- a/block.c >>> +++ b/block.c >>> @@ -434,6 +434,8 @@ int get_tmp_filename(char *filename, int size) >>> } >>> fd = mkstemp(filename); >>> if (fd < 0 || close(fd)) { >>> + fprintf(stderr, "Could not create temporary snapshot in %s directory: " >>> + "%s\n", tmpdir, strerror(errno)); >> >> The error message is fine for fd < 0 but not for close(0) != 0. Also, >> close(2) is allowed to change errno (even on success) so this is not >> portable and could clobber the errno value. > > I don't think this error message is fine in get_tmp_filename(). This > function happens to be used for temporary snapshots, but this is not > part of its interface. Today it's also used in vvfat and there the > message would only be confusing. Other use cases may be introduced in > the future. Sorry, I forgot that get_tmp_filename() is a public interface. > > If you want to introduce an error message, do it in the caller. > > Kevin
diff --git a/block.c b/block.c index 470bdcc..c9e5140 100644 --- a/block.c +++ b/block.c @@ -434,6 +434,8 @@ int get_tmp_filename(char *filename, int size) } fd = mkstemp(filename); if (fd < 0 || close(fd)) { + fprintf(stderr, "Could not create temporary snapshot in %s directory: " + "%s\n", tmpdir, strerror(errno)); return -errno; } return 0;