| Message ID | 1383678213-2497-1-git-send-email-mreitz@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 11/05/2013 12:03 PM, Max Reitz wrote: > error_setg_errno() may overwrite errno; therefore, its value should be > read before calling that function and not afterwards. > > Signed-off-by: Max Reitz <mreitz@redhat.com> > --- > block.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) Reviewed-by: Eric Blake <eblake@redhat.com> Still, wouldn't it be easier to patch error_setg_errno (and friends) to guarantee that errno is unchanged on exit compared to its value on entrance, rather than having to audit for other mistakes like this? > > diff --git a/block.c b/block.c > index 58efb5b..0e96a22 100644 > --- a/block.c > +++ b/block.c > @@ -1084,8 +1084,8 @@ int bdrv_open(BlockDriverState *bs, const char *filename, QDict *options, > snprintf(backing_filename, sizeof(backing_filename), > "%s", filename); > } else if (!realpath(filename, backing_filename)) { > - error_setg_errno(errp, errno, "Could not resolve path '%s'", filename); > ret = -errno; > + error_setg_errno(errp, errno, "Could not resolve path '%s'", filename); > goto fail; > } > >
Le Tuesday 05 Nov 2013 à 20:03:33 (+0100), Max Reitz a écrit : > error_setg_errno() may overwrite errno; therefore, its value should be > read before calling that function and not afterwards. > > Signed-off-by: Max Reitz <mreitz@redhat.com> > --- > block.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/block.c b/block.c > index 58efb5b..0e96a22 100644 > --- a/block.c > +++ b/block.c > @@ -1084,8 +1084,8 @@ int bdrv_open(BlockDriverState *bs, const char *filename, QDict *options, > snprintf(backing_filename, sizeof(backing_filename), > "%s", filename); > } else if (!realpath(filename, backing_filename)) { > - error_setg_errno(errp, errno, "Could not resolve path '%s'", filename); > ret = -errno; > + error_setg_errno(errp, errno, "Could not resolve path '%s'", filename); > goto fail; > } > > -- > 1.8.4.2 > Reviewed-by: Benoit Canet <benoit@irqsave.net> >
On Tue, Nov 05, 2013 at 08:03:33PM +0100, Max Reitz wrote: > error_setg_errno() may overwrite errno; therefore, its value should be > read before calling that function and not afterwards. > > Signed-off-by: Max Reitz <mreitz@redhat.com> > --- > block.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) Thanks, applied to my block tree: https://github.com/stefanha/qemu/commits/block Stefan
On Tue, Nov 05, 2013 at 12:26:41PM -0700, Eric Blake wrote: > On 11/05/2013 12:03 PM, Max Reitz wrote: > > error_setg_errno() may overwrite errno; therefore, its value should be > > read before calling that function and not afterwards. > > > > Signed-off-by: Max Reitz <mreitz@redhat.com> > > --- > > block.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > Reviewed-by: Eric Blake <eblake@redhat.com> > > Still, wouldn't it be easier to patch error_setg_errno (and friends) to > guarantee that errno is unchanged on exit compared to its value on > entrance, rather than having to audit for other mistakes like this? Agreed, that can be done as a separate patch. Stefan
diff --git a/block.c b/block.c index 58efb5b..0e96a22 100644 --- a/block.c +++ b/block.c @@ -1084,8 +1084,8 @@ int bdrv_open(BlockDriverState *bs, const char *filename, QDict *options, snprintf(backing_filename, sizeof(backing_filename), "%s", filename); } else if (!realpath(filename, backing_filename)) { - error_setg_errno(errp, errno, "Could not resolve path '%s'", filename); ret = -errno; + error_setg_errno(errp, errno, "Could not resolve path '%s'", filename); goto fail; }
error_setg_errno() may overwrite errno; therefore, its value should be read before calling that function and not afterwards. Signed-off-by: Max Reitz <mreitz@redhat.com> --- block.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)