| Message ID | 1498637057-23221-1-git-send-email-hare@suse.de (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Delegated to: | Mike Snitzer |
| Headers | show |
On Wed, Jun 28 2017 at 4:04P -0400, Hannes Reinecke <hare@suse.de> wrote: > __rdev_sectors() might be called for an invalid/non-existing > RAID set during raid_ctr(), which is perfectly fine and no > reason to panic. > > Signed-off-by: Hannes Reinecke <hare@suse.com> > --- > drivers/md/dm-raid.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/md/dm-raid.c b/drivers/md/dm-raid.c > index 7d89322..9bbb596 100644 > --- a/drivers/md/dm-raid.c > +++ b/drivers/md/dm-raid.c > @@ -1571,7 +1571,8 @@ static sector_t __rdev_sectors(struct raid_set *rs) > return rdev->sectors; > } > > - BUG(); /* Constructor ensures we got some. */ > + /* No valid raid devices */ > + return 0; > } > > /* Calculate the sectors per device and per array used for @rs */ > -- > 1.8.5.6 > The use of BUG() is certainly suspect. BUG() is very rarely the right answer. But I don't think returning 0 makes sense for all __rdev_sectors() callers, e.g.: rs_setup_recovery() __rdev_sectors() could easily be made to check if ti->error is passed as a non-NULL pointer.. if so set *error, return 0, have ctr check for 0, and gracefully fail ctr by returning the ti->error that __rdev_sectors() set. If the error pointer passed to __rdev_sectors() is NULL, resort to BUG() still? Would really like to avoid BUG().. but I'll defer to Heinz on what might be a better response. Alternatively, you could look to see where "Constructor ensures we got some." and fix the fact that it clearly isn't ensuring as much for your case? Mike -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
Hi Hannes, thanks for your patch. BUG() is intentional there to catch what I've already fixed with patch "[dm-devel] [PATCH] dm raid: fix oops on upgrading to extended superblock format" on 06/23/2017. IOW: returning 0 is never right and would hide bugs like that one fixed. Heinz On 06/28/2017 10:04 AM, Hannes Reinecke wrote: > __rdev_sectors() might be called for an invalid/non-existing > RAID set during raid_ctr(), which is perfectly fine and no > reason to panic. > > Signed-off-by: Hannes Reinecke <hare@suse.com> > --- > drivers/md/dm-raid.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/md/dm-raid.c b/drivers/md/dm-raid.c > index 7d89322..9bbb596 100644 > --- a/drivers/md/dm-raid.c > +++ b/drivers/md/dm-raid.c > @@ -1571,7 +1571,8 @@ static sector_t __rdev_sectors(struct raid_set *rs) > return rdev->sectors; > } > > - BUG(); /* Constructor ensures we got some. */ > + /* No valid raid devices */ > + return 0; > } > > /* Calculate the sectors per device and per array used for @rs */ -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
On Thu, Jun 29, 2017 at 04:09:38PM +0200, Heinz Mauelshagen wrote: > > Hi Hannes, > > thanks for your patch. > > BUG() is intentional there to catch what I've already fixed with patch > > "[dm-devel] [PATCH] dm raid: fix oops on upgrading to extended superblock > format" > > on 06/23/2017. > > IOW: returning 0 is never right and would hide bugs like that one fixed. OTOH panicing systems to catch errors which could've been mitigted is never right as well. Not a single time. Never. BUG() and BUG_ON() are not for error handling.
On 06/30/2017 09:33 AM, Johannes Thumshirn wrote: > On Thu, Jun 29, 2017 at 04:09:38PM +0200, Heinz Mauelshagen wrote: >> Hi Hannes, >> >> thanks for your patch. >> >> BUG() is intentional there to catch what I've already fixed with patch >> >> "[dm-devel] [PATCH] dm raid: fix oops on upgrading to extended superblock >> format" >> >> on 06/23/2017. >> >> IOW: returning 0 is never right and would hide bugs like that one fixed. > OTOH panicing systems to catch errors which could've been mitigted is > never right as well. Not a single time. Never. BUG() and BUG_ON() are not > for error handling. Point taken. We'll change that. > -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
diff --git a/drivers/md/dm-raid.c b/drivers/md/dm-raid.c index 7d89322..9bbb596 100644 --- a/drivers/md/dm-raid.c +++ b/drivers/md/dm-raid.c @@ -1571,7 +1571,8 @@ static sector_t __rdev_sectors(struct raid_set *rs) return rdev->sectors; } - BUG(); /* Constructor ensures we got some. */ + /* No valid raid devices */ + return 0; } /* Calculate the sectors per device and per array used for @rs */
__rdev_sectors() might be called for an invalid/non-existing RAID set during raid_ctr(), which is perfectly fine and no reason to panic. Signed-off-by: Hannes Reinecke <hare@suse.com> --- drivers/md/dm-raid.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)