| Message ID | 1380362114-29476-1-git-send-email-sw@weilnetz.de |
|---|---|
| State | Accepted |
| Headers | show |
28.09.2013 13:55, Stefan Weil wrote: > blockdev.c:1929:13: warning: Value stored to 'ret' is never read > ret = 0; > ^ ~ Applied to the trivial patches queue. > Signed-off-by: Stefan Weil <sw@weilnetz.de> > --- > blockdev.c | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/blockdev.c b/blockdev.c > index 8aa66a9..8c83f6f 100644 > --- a/blockdev.c > +++ b/blockdev.c > @@ -1926,7 +1926,6 @@ void qmp_drive_mirror(const char *device, const char *target, > } else { > switch (mode) { > case NEW_IMAGE_MODE_EXISTING: > - ret = 0; > break; While this one is obviously unused assignment, there's on more usage of `ret' variable in this function, -- it is to store the return value from bdrv_open(): ret = bdrv_open(target_bs, target, NULL, flags | BDRV_O_NO_BACKING, drv, &local_err); if (ret < 0) {... What's the rule about converting that into if() ? Thanks, /mjt
Am 29.09.2013 21:44, schrieb Michael Tokarev: > 28.09.2013 13:55, Stefan Weil wrote: [...] >> diff --git a/blockdev.c b/blockdev.c >> index 8aa66a9..8c83f6f 100644 >> --- a/blockdev.c >> +++ b/blockdev.c >> @@ -1926,7 +1926,6 @@ void qmp_drive_mirror(const char *device, const >> char *target, >> } else { >> switch (mode) { >> case NEW_IMAGE_MODE_EXISTING: >> - ret = 0; >> break; > > While this one is obviously unused assignment, > there's on more usage of `ret' variable in this > function, -- it is to store the return value > from bdrv_open(): > > ret = bdrv_open(target_bs, target, NULL, flags | > BDRV_O_NO_BACKING, drv, > &local_err); > if (ret < 0) {... > > What's the rule about converting that into if() ? > > Thanks, > > /mjt Is there a rule for cases like that? This pattern is very common in QEMU code (several occurrences in blockdev.c). Should we eliminate the 'ret' variable? I don't think it's worth the effort. Stefan
Am 29.09.2013 um 22:15 hat Stefan Weil geschrieben: > Am 29.09.2013 21:44, schrieb Michael Tokarev: > > 28.09.2013 13:55, Stefan Weil wrote: > [...] > >> diff --git a/blockdev.c b/blockdev.c > >> index 8aa66a9..8c83f6f 100644 > >> --- a/blockdev.c > >> +++ b/blockdev.c > >> @@ -1926,7 +1926,6 @@ void qmp_drive_mirror(const char *device, const > >> char *target, > >> } else { > >> switch (mode) { > >> case NEW_IMAGE_MODE_EXISTING: > >> - ret = 0; > >> break; > > > > While this one is obviously unused assignment, > > there's on more usage of `ret' variable in this > > function, -- it is to store the return value > > from bdrv_open(): > > > > ret = bdrv_open(target_bs, target, NULL, flags | > > BDRV_O_NO_BACKING, drv, > > &local_err); > > if (ret < 0) {... > > > > What's the rule about converting that into if() ? > > > > Thanks, > > > > /mjt > > Is there a rule for cases like that? This pattern is very common in QEMU > code > (several occurrences in blockdev.c). Should we eliminate the 'ret' variable? > I don't think it's worth the effort. And actually I think removing it would make the code worse (less readable). Kevin
On Sun, Sep 29, 2013 at 10:15:10PM +0200, Stefan Weil wrote: > Am 29.09.2013 21:44, schrieb Michael Tokarev: > > 28.09.2013 13:55, Stefan Weil wrote: > [...] > >> diff --git a/blockdev.c b/blockdev.c > >> index 8aa66a9..8c83f6f 100644 > >> --- a/blockdev.c > >> +++ b/blockdev.c > >> @@ -1926,7 +1926,6 @@ void qmp_drive_mirror(const char *device, const > >> char *target, > >> } else { > >> switch (mode) { > >> case NEW_IMAGE_MODE_EXISTING: > >> - ret = 0; > >> break; > > > > While this one is obviously unused assignment, > > there's on more usage of `ret' variable in this > > function, -- it is to store the return value > > from bdrv_open(): > > > > ret = bdrv_open(target_bs, target, NULL, flags | > > BDRV_O_NO_BACKING, drv, > > &local_err); > > if (ret < 0) {... > > > > What's the rule about converting that into if() ? > > > > Thanks, > > > > /mjt > > Is there a rule for cases like that? This pattern is very common in QEMU > code > (several occurrences in blockdev.c). Should we eliminate the 'ret' variable? > I don't think it's worth the effort. Me neither. There is a minor style difference between putting everything into the if statement and using a variable to split up a potentially complicated if statement. Both approaches are usually okay. Stefan
diff --git a/blockdev.c b/blockdev.c index 8aa66a9..8c83f6f 100644 --- a/blockdev.c +++ b/blockdev.c @@ -1926,7 +1926,6 @@ void qmp_drive_mirror(const char *device, const char *target, } else { switch (mode) { case NEW_IMAGE_MODE_EXISTING: - ret = 0; break; case NEW_IMAGE_MODE_ABSOLUTE_PATHS: /* create new image with backing file */
blockdev.c:1929:13: warning: Value stored to 'ret' is never read ret = 0; ^ ~ Signed-off-by: Stefan Weil <sw@weilnetz.de> --- blockdev.c | 1 - 1 file changed, 1 deletion(-)