| Message ID | 155839424514.68606.14562327454853103352.stgit@magnolia (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | xfsprogs-5.1: fix various problems | expand |
On 5/20/19 6:17 PM, Darrick J. Wong wrote: > From: Darrick J. Wong <darrick.wong@oracle.com> > > Fix the return types of non-predicate bitmap functions to return the > usual negative error codes instead of the "moveon" boolean. This seems much better, though how did you decide on negative error codes? They are usual for the kernel, but in userspace we have kind of a mishmash, even in libfrog. File Function Line 0 libfrog/paths.c fs_table_insert 176 error = ENOMEM; 1 libfrog/paths.c fs_extract_mount_options 354 return ENOMEM; 2 libfrog/radix-tree.c radix_tree_extend 135 return -ENOMEM; 3 libfrog/radix-tree.c radix_tree_insert 188 return -ENOMEM; 4 libfrog/workqueue.c workqueue_add 110 return ENOMEM; 3 libfrog/paths.c fs_table_initialise_mounts 384 return ENOENT; 4 libfrog/paths.c fs_table_initialise_projects 489 error = ENOENT; 5 libfrog/paths.c fs_table_insert_project_path 560 error = ENOENT; > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com> > --- > include/bitmap.h | 8 +++-- > libfrog/bitmap.c | 86 ++++++++++++++++++++++++++---------------------------- > repair/rmap.c | 18 ++++++++--- > scrub/phase6.c | 18 ++++------- > 4 files changed, 65 insertions(+), 65 deletions(-) > > > diff --git a/include/bitmap.h b/include/bitmap.h > index e29a4335..99a2fb23 100644 > --- a/include/bitmap.h > +++ b/include/bitmap.h > @@ -11,11 +11,11 @@ struct bitmap { > struct avl64tree_desc *bt_tree; > }; > > -bool bitmap_init(struct bitmap **bmap); > +int bitmap_init(struct bitmap **bmap); > void bitmap_free(struct bitmap **bmap); > -bool bitmap_set(struct bitmap *bmap, uint64_t start, uint64_t length); > -bool bitmap_iterate(struct bitmap *bmap, > - bool (*fn)(uint64_t, uint64_t, void *), void *arg); > +int bitmap_set(struct bitmap *bmap, uint64_t start, uint64_t length); > +int bitmap_iterate(struct bitmap *bmap, int (*fn)(uint64_t, uint64_t, void *), > + void *arg); > bool bitmap_test(struct bitmap *bmap, uint64_t start, > uint64_t len); > bool bitmap_empty(struct bitmap *bmap); > diff --git a/libfrog/bitmap.c b/libfrog/bitmap.c > index 450ebe0a..4dafc4c9 100644 > --- a/libfrog/bitmap.c > +++ b/libfrog/bitmap.c > @@ -66,7 +66,7 @@ static struct avl64ops bitmap_ops = { > }; > > /* Initialize a bitmap. */ > -bool > +int > bitmap_init( > struct bitmap **bmapp) > { > @@ -74,18 +74,18 @@ bitmap_init( > > bmap = calloc(1, sizeof(struct bitmap)); > if (!bmap) > - return false; > + return -ENOMEM; > bmap->bt_tree = malloc(sizeof(struct avl64tree_desc)); > if (!bmap->bt_tree) { > free(bmap); > - return false; > + return -ENOMEM; > } > > pthread_mutex_init(&bmap->bt_lock, NULL); > avl64_init_tree(bmap->bt_tree, &bitmap_ops); > *bmapp = bmap; > > - return true; > + return 0; > } > > /* Free a bitmap. */ > @@ -127,8 +127,31 @@ bitmap_node_init( > return ext; > } > > +/* Create a new bitmap node and insert it. */ > +static inline int > +__bitmap_insert( > + struct bitmap *bmap, > + uint64_t start, > + uint64_t length) > +{ > + struct bitmap_node *ext; > + struct avl64node *node; > + > + ext = bitmap_node_init(start, length); > + if (!ext) > + return -ENOMEM; > + > + node = avl64_insert(bmap->bt_tree, &ext->btn_node); > + if (node == NULL) { > + free(ext); > + return -EEXIST; > + } > + > + return 0; > +} > + > /* Set a region of bits (locked). */ > -static bool > +static int > __bitmap_set( > struct bitmap *bmap, > uint64_t start, > @@ -142,28 +165,14 @@ __bitmap_set( > struct bitmap_node *ext; > uint64_t new_start; > uint64_t new_length; > - struct avl64node *node; > - bool res = true; > > /* Find any existing nodes adjacent or within that range. */ > avl64_findranges(bmap->bt_tree, start - 1, start + length + 1, > &firstn, &lastn); > > /* Nothing, just insert a new extent. */ > - if (firstn == NULL && lastn == NULL) { > - ext = bitmap_node_init(start, length); > - if (!ext) > - return false; > - > - node = avl64_insert(bmap->bt_tree, &ext->btn_node); > - if (node == NULL) { > - free(ext); > - errno = EEXIST; > - return false; > - } > - > - return true; > - } > + if (firstn == NULL && lastn == NULL) > + return __bitmap_insert(bmap, start, length); > > assert(firstn != NULL && lastn != NULL); > new_start = start; > @@ -175,7 +184,7 @@ __bitmap_set( > /* Bail if the new extent is contained within an old one. */ > if (ext->btn_start <= start && > ext->btn_start + ext->btn_length >= start + length) > - return res; > + return 0; > > /* Check for overlapping and adjacent extents. */ > if (ext->btn_start + ext->btn_length >= start || > @@ -195,28 +204,17 @@ __bitmap_set( > } > } > > - ext = bitmap_node_init(new_start, new_length); > - if (!ext) > - return false; > - > - node = avl64_insert(bmap->bt_tree, &ext->btn_node); > - if (node == NULL) { > - free(ext); > - errno = EEXIST; > - return false; > - } > - > - return res; > + return __bitmap_insert(bmap, new_start, new_length); > } > > /* Set a region of bits. */ > -bool > +int > bitmap_set( > struct bitmap *bmap, > uint64_t start, > uint64_t length) > { > - bool res; > + int res; > > pthread_mutex_lock(&bmap->bt_lock); > res = __bitmap_set(bmap, start, length); > @@ -308,26 +306,26 @@ bitmap_clear( > > #ifdef DEBUG > /* Iterate the set regions of this bitmap. */ > -bool > +int > bitmap_iterate( > struct bitmap *bmap, > - bool (*fn)(uint64_t, uint64_t, void *), > + int (*fn)(uint64_t, uint64_t, void *), > void *arg) > { > struct avl64node *node; > struct bitmap_node *ext; > - bool moveon = true; > + int error = 0; > > pthread_mutex_lock(&bmap->bt_lock); > avl_for_each(bmap->bt_tree, node) { > ext = container_of(node, struct bitmap_node, btn_node); > - moveon = fn(ext->btn_start, ext->btn_length, arg); > - if (!moveon) > + error = fn(ext->btn_start, ext->btn_length, arg); > + if (error) > break; > } > pthread_mutex_unlock(&bmap->bt_lock); > > - return moveon; > + return error; > } > #endif > > @@ -372,14 +370,14 @@ bitmap_empty( > } > > #ifdef DEBUG > -static bool > +static int > bitmap_dump_fn( > uint64_t startblock, > uint64_t blockcount, > void *arg) > { > printf("%"PRIu64":%"PRIu64"\n", startblock, blockcount); > - return true; > + return 0; > } > > /* Dump bitmap. */ > diff --git a/repair/rmap.c b/repair/rmap.c > index 19cceca3..47828a06 100644 > --- a/repair/rmap.c > +++ b/repair/rmap.c > @@ -490,16 +490,22 @@ rmap_store_ag_btree_rec( > error = init_slab_cursor(ag_rmap->ar_raw_rmaps, rmap_compare, &rm_cur); > if (error) > goto err; > - if (!bitmap_init(&own_ag_bitmap)) { > - error = -ENOMEM; > + error = -bitmap_init(&own_ag_bitmap); > + if (error) > goto err_slab; > - } > while ((rm_rec = pop_slab_cursor(rm_cur)) != NULL) { > if (rm_rec->rm_owner != XFS_RMAP_OWN_AG) > continue; > - if (!bitmap_set(own_ag_bitmap, rm_rec->rm_startblock, > - rm_rec->rm_blockcount)) { > - error = EFSCORRUPTED; > + error = -bitmap_set(own_ag_bitmap, rm_rec->rm_startblock, > + rm_rec->rm_blockcount); > + if (error) { > + /* > + * If this range is already set, then the incore rmap > + * records for the AG free space btrees overlap and > + * we're toast because that is not allowed. > + */ > + if (error == EEXIST) > + error = EFSCORRUPTED; > goto err_slab; > } > } > diff --git a/scrub/phase6.c b/scrub/phase6.c > index 4b25f3bb..66e6451c 100644 > --- a/scrub/phase6.c > +++ b/scrub/phase6.c > @@ -341,7 +341,6 @@ xfs_check_rmap_ioerr( > struct media_verify_state *vs = arg; > struct bitmap *tree; > dev_t dev; > - bool moveon; > > dev = xfs_disk_to_dev(ctx, disk); > > @@ -356,8 +355,8 @@ xfs_check_rmap_ioerr( > else > tree = NULL; > if (tree) { > - moveon = bitmap_set(tree, start, length); > - if (!moveon) > + errno = -bitmap_set(tree, start, length); > + if (errno) > str_errno(ctx, ctx->mntpoint); > } > > @@ -454,16 +453,16 @@ xfs_scan_blocks( > struct scrub_ctx *ctx) > { > struct media_verify_state vs = { NULL }; > - bool moveon; > + bool moveon = false; > > - moveon = bitmap_init(&vs.d_bad); > - if (!moveon) { > + errno = -bitmap_init(&vs.d_bad); > + if (errno) { > str_errno(ctx, ctx->mntpoint); > goto out; > } > > - moveon = bitmap_init(&vs.r_bad); > - if (!moveon) { > + errno = -bitmap_init(&vs.r_bad); > + if (errno) { > str_errno(ctx, ctx->mntpoint); > goto out_dbad; > } > @@ -472,7 +471,6 @@ xfs_scan_blocks( > ctx->geo.blocksize, xfs_check_rmap_ioerr, > scrub_nproc(ctx)); > if (!vs.rvp_data) { > - moveon = false; > str_info(ctx, ctx->mntpoint, > _("Could not create data device media verifier.")); > goto out_rbad; > @@ -482,7 +480,6 @@ _("Could not create data device media verifier.")); > ctx->geo.blocksize, xfs_check_rmap_ioerr, > scrub_nproc(ctx)); > if (!vs.rvp_log) { > - moveon = false; > str_info(ctx, ctx->mntpoint, > _("Could not create log device media verifier.")); > goto out_datapool; > @@ -493,7 +490,6 @@ _("Could not create data device media verifier.")); > ctx->geo.blocksize, xfs_check_rmap_ioerr, > scrub_nproc(ctx)); > if (!vs.rvp_realtime) { > - moveon = false; > str_info(ctx, ctx->mntpoint, > _("Could not create realtime device media verifier.")); > goto out_logpool; >
On Tue, May 21, 2019 at 11:54:18AM -0500, Eric Sandeen wrote: > On 5/20/19 6:17 PM, Darrick J. Wong wrote: > > From: Darrick J. Wong <darrick.wong@oracle.com> > > > > Fix the return types of non-predicate bitmap functions to return the > > usual negative error codes instead of the "moveon" boolean. > > This seems much better, though how did you decide on negative > error codes? They are usual for the kernel, but in userspace > we have kind of a mishmash, even in libfrog. > > File Function Line > 0 libfrog/paths.c fs_table_insert 176 error = ENOMEM; > 1 libfrog/paths.c fs_extract_mount_options 354 return ENOMEM; > 2 libfrog/radix-tree.c radix_tree_extend 135 return -ENOMEM; > 3 libfrog/radix-tree.c radix_tree_insert 188 return -ENOMEM; > 4 libfrog/workqueue.c workqueue_add 110 return ENOMEM; > > 3 libfrog/paths.c fs_table_initialise_mounts 384 return ENOENT; > 4 libfrog/paths.c fs_table_initialise_projects 489 error = ENOENT; > 5 libfrog/paths.c fs_table_insert_project_path 560 error = ENOENT; Blindly copying libxfs style. :) I see your point about being consistent within libfrog but OTOH it's messy that we're not consistent across the various xfsprogs libraries. Uhm.... I'll change it if you want. --D > > > > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com> > > --- > > include/bitmap.h | 8 +++-- > > libfrog/bitmap.c | 86 ++++++++++++++++++++++++++---------------------------- > > repair/rmap.c | 18 ++++++++--- > > scrub/phase6.c | 18 ++++------- > > 4 files changed, 65 insertions(+), 65 deletions(-) > > > > > > diff --git a/include/bitmap.h b/include/bitmap.h > > index e29a4335..99a2fb23 100644 > > --- a/include/bitmap.h > > +++ b/include/bitmap.h > > @@ -11,11 +11,11 @@ struct bitmap { > > struct avl64tree_desc *bt_tree; > > }; > > > > -bool bitmap_init(struct bitmap **bmap); > > +int bitmap_init(struct bitmap **bmap); > > void bitmap_free(struct bitmap **bmap); > > -bool bitmap_set(struct bitmap *bmap, uint64_t start, uint64_t length); > > -bool bitmap_iterate(struct bitmap *bmap, > > - bool (*fn)(uint64_t, uint64_t, void *), void *arg); > > +int bitmap_set(struct bitmap *bmap, uint64_t start, uint64_t length); > > +int bitmap_iterate(struct bitmap *bmap, int (*fn)(uint64_t, uint64_t, void *), > > + void *arg); > > bool bitmap_test(struct bitmap *bmap, uint64_t start, > > uint64_t len); > > bool bitmap_empty(struct bitmap *bmap); > > diff --git a/libfrog/bitmap.c b/libfrog/bitmap.c > > index 450ebe0a..4dafc4c9 100644 > > --- a/libfrog/bitmap.c > > +++ b/libfrog/bitmap.c > > @@ -66,7 +66,7 @@ static struct avl64ops bitmap_ops = { > > }; > > > > /* Initialize a bitmap. */ > > -bool > > +int > > bitmap_init( > > struct bitmap **bmapp) > > { > > @@ -74,18 +74,18 @@ bitmap_init( > > > > bmap = calloc(1, sizeof(struct bitmap)); > > if (!bmap) > > - return false; > > + return -ENOMEM; > > bmap->bt_tree = malloc(sizeof(struct avl64tree_desc)); > > if (!bmap->bt_tree) { > > free(bmap); > > - return false; > > + return -ENOMEM; > > } > > > > pthread_mutex_init(&bmap->bt_lock, NULL); > > avl64_init_tree(bmap->bt_tree, &bitmap_ops); > > *bmapp = bmap; > > > > - return true; > > + return 0; > > } > > > > /* Free a bitmap. */ > > @@ -127,8 +127,31 @@ bitmap_node_init( > > return ext; > > } > > > > +/* Create a new bitmap node and insert it. */ > > +static inline int > > +__bitmap_insert( > > + struct bitmap *bmap, > > + uint64_t start, > > + uint64_t length) > > +{ > > + struct bitmap_node *ext; > > + struct avl64node *node; > > + > > + ext = bitmap_node_init(start, length); > > + if (!ext) > > + return -ENOMEM; > > + > > + node = avl64_insert(bmap->bt_tree, &ext->btn_node); > > + if (node == NULL) { > > + free(ext); > > + return -EEXIST; > > + } > > + > > + return 0; > > +} > > + > > /* Set a region of bits (locked). */ > > -static bool > > +static int > > __bitmap_set( > > struct bitmap *bmap, > > uint64_t start, > > @@ -142,28 +165,14 @@ __bitmap_set( > > struct bitmap_node *ext; > > uint64_t new_start; > > uint64_t new_length; > > - struct avl64node *node; > > - bool res = true; > > > > /* Find any existing nodes adjacent or within that range. */ > > avl64_findranges(bmap->bt_tree, start - 1, start + length + 1, > > &firstn, &lastn); > > > > /* Nothing, just insert a new extent. */ > > - if (firstn == NULL && lastn == NULL) { > > - ext = bitmap_node_init(start, length); > > - if (!ext) > > - return false; > > - > > - node = avl64_insert(bmap->bt_tree, &ext->btn_node); > > - if (node == NULL) { > > - free(ext); > > - errno = EEXIST; > > - return false; > > - } > > - > > - return true; > > - } > > + if (firstn == NULL && lastn == NULL) > > + return __bitmap_insert(bmap, start, length); > > > > assert(firstn != NULL && lastn != NULL); > > new_start = start; > > @@ -175,7 +184,7 @@ __bitmap_set( > > /* Bail if the new extent is contained within an old one. */ > > if (ext->btn_start <= start && > > ext->btn_start + ext->btn_length >= start + length) > > - return res; > > + return 0; > > > > /* Check for overlapping and adjacent extents. */ > > if (ext->btn_start + ext->btn_length >= start || > > @@ -195,28 +204,17 @@ __bitmap_set( > > } > > } > > > > - ext = bitmap_node_init(new_start, new_length); > > - if (!ext) > > - return false; > > - > > - node = avl64_insert(bmap->bt_tree, &ext->btn_node); > > - if (node == NULL) { > > - free(ext); > > - errno = EEXIST; > > - return false; > > - } > > - > > - return res; > > + return __bitmap_insert(bmap, new_start, new_length); > > } > > > > /* Set a region of bits. */ > > -bool > > +int > > bitmap_set( > > struct bitmap *bmap, > > uint64_t start, > > uint64_t length) > > { > > - bool res; > > + int res; > > > > pthread_mutex_lock(&bmap->bt_lock); > > res = __bitmap_set(bmap, start, length); > > @@ -308,26 +306,26 @@ bitmap_clear( > > > > #ifdef DEBUG > > /* Iterate the set regions of this bitmap. */ > > -bool > > +int > > bitmap_iterate( > > struct bitmap *bmap, > > - bool (*fn)(uint64_t, uint64_t, void *), > > + int (*fn)(uint64_t, uint64_t, void *), > > void *arg) > > { > > struct avl64node *node; > > struct bitmap_node *ext; > > - bool moveon = true; > > + int error = 0; > > > > pthread_mutex_lock(&bmap->bt_lock); > > avl_for_each(bmap->bt_tree, node) { > > ext = container_of(node, struct bitmap_node, btn_node); > > - moveon = fn(ext->btn_start, ext->btn_length, arg); > > - if (!moveon) > > + error = fn(ext->btn_start, ext->btn_length, arg); > > + if (error) > > break; > > } > > pthread_mutex_unlock(&bmap->bt_lock); > > > > - return moveon; > > + return error; > > } > > #endif > > > > @@ -372,14 +370,14 @@ bitmap_empty( > > } > > > > #ifdef DEBUG > > -static bool > > +static int > > bitmap_dump_fn( > > uint64_t startblock, > > uint64_t blockcount, > > void *arg) > > { > > printf("%"PRIu64":%"PRIu64"\n", startblock, blockcount); > > - return true; > > + return 0; > > } > > > > /* Dump bitmap. */ > > diff --git a/repair/rmap.c b/repair/rmap.c > > index 19cceca3..47828a06 100644 > > --- a/repair/rmap.c > > +++ b/repair/rmap.c > > @@ -490,16 +490,22 @@ rmap_store_ag_btree_rec( > > error = init_slab_cursor(ag_rmap->ar_raw_rmaps, rmap_compare, &rm_cur); > > if (error) > > goto err; > > - if (!bitmap_init(&own_ag_bitmap)) { > > - error = -ENOMEM; > > + error = -bitmap_init(&own_ag_bitmap); > > + if (error) > > goto err_slab; > > - } > > while ((rm_rec = pop_slab_cursor(rm_cur)) != NULL) { > > if (rm_rec->rm_owner != XFS_RMAP_OWN_AG) > > continue; > > - if (!bitmap_set(own_ag_bitmap, rm_rec->rm_startblock, > > - rm_rec->rm_blockcount)) { > > - error = EFSCORRUPTED; > > + error = -bitmap_set(own_ag_bitmap, rm_rec->rm_startblock, > > + rm_rec->rm_blockcount); > > + if (error) { > > + /* > > + * If this range is already set, then the incore rmap > > + * records for the AG free space btrees overlap and > > + * we're toast because that is not allowed. > > + */ > > + if (error == EEXIST) > > + error = EFSCORRUPTED; > > goto err_slab; > > } > > } > > diff --git a/scrub/phase6.c b/scrub/phase6.c > > index 4b25f3bb..66e6451c 100644 > > --- a/scrub/phase6.c > > +++ b/scrub/phase6.c > > @@ -341,7 +341,6 @@ xfs_check_rmap_ioerr( > > struct media_verify_state *vs = arg; > > struct bitmap *tree; > > dev_t dev; > > - bool moveon; > > > > dev = xfs_disk_to_dev(ctx, disk); > > > > @@ -356,8 +355,8 @@ xfs_check_rmap_ioerr( > > else > > tree = NULL; > > if (tree) { > > - moveon = bitmap_set(tree, start, length); > > - if (!moveon) > > + errno = -bitmap_set(tree, start, length); > > + if (errno) > > str_errno(ctx, ctx->mntpoint); > > } > > > > @@ -454,16 +453,16 @@ xfs_scan_blocks( > > struct scrub_ctx *ctx) > > { > > struct media_verify_state vs = { NULL }; > > - bool moveon; > > + bool moveon = false; > > > > - moveon = bitmap_init(&vs.d_bad); > > - if (!moveon) { > > + errno = -bitmap_init(&vs.d_bad); > > + if (errno) { > > str_errno(ctx, ctx->mntpoint); > > goto out; > > } > > > > - moveon = bitmap_init(&vs.r_bad); > > - if (!moveon) { > > + errno = -bitmap_init(&vs.r_bad); > > + if (errno) { > > str_errno(ctx, ctx->mntpoint); > > goto out_dbad; > > } > > @@ -472,7 +471,6 @@ xfs_scan_blocks( > > ctx->geo.blocksize, xfs_check_rmap_ioerr, > > scrub_nproc(ctx)); > > if (!vs.rvp_data) { > > - moveon = false; > > str_info(ctx, ctx->mntpoint, > > _("Could not create data device media verifier.")); > > goto out_rbad; > > @@ -482,7 +480,6 @@ _("Could not create data device media verifier.")); > > ctx->geo.blocksize, xfs_check_rmap_ioerr, > > scrub_nproc(ctx)); > > if (!vs.rvp_log) { > > - moveon = false; > > str_info(ctx, ctx->mntpoint, > > _("Could not create log device media verifier.")); > > goto out_datapool; > > @@ -493,7 +490,6 @@ _("Could not create data device media verifier.")); > > ctx->geo.blocksize, xfs_check_rmap_ioerr, > > scrub_nproc(ctx)); > > if (!vs.rvp_realtime) { > > - moveon = false; > > str_info(ctx, ctx->mntpoint, > > _("Could not create realtime device media verifier.")); > > goto out_logpool; > >
On 5/21/19 12:01 PM, Darrick J. Wong wrote: > On Tue, May 21, 2019 at 11:54:18AM -0500, Eric Sandeen wrote: >> On 5/20/19 6:17 PM, Darrick J. Wong wrote: >>> From: Darrick J. Wong <darrick.wong@oracle.com> >>> >>> Fix the return types of non-predicate bitmap functions to return the >>> usual negative error codes instead of the "moveon" boolean. >> >> This seems much better, though how did you decide on negative >> error codes? They are usual for the kernel, but in userspace >> we have kind of a mishmash, even in libfrog. >> >> File Function Line >> 0 libfrog/paths.c fs_table_insert 176 error = ENOMEM; >> 1 libfrog/paths.c fs_extract_mount_options 354 return ENOMEM; >> 2 libfrog/radix-tree.c radix_tree_extend 135 return -ENOMEM; >> 3 libfrog/radix-tree.c radix_tree_insert 188 return -ENOMEM; >> 4 libfrog/workqueue.c workqueue_add 110 return ENOMEM; >> >> 3 libfrog/paths.c fs_table_initialise_mounts 384 return ENOENT; >> 4 libfrog/paths.c fs_table_initialise_projects 489 error = ENOENT; >> 5 libfrog/paths.c fs_table_insert_project_path 560 error = ENOENT; > > Blindly copying libxfs style. :) > > I see your point about being consistent within libfrog but OTOH it's > messy that we're not consistent across the various xfsprogs libraries. > > Uhm.... I'll change it if you want. If I were king (am I king?) I'd say that kernel code does negative values, userspace does positive, and we handle differences at the interface. Mostly I just want it to be predictable and consistent... AFAICT everything in libfrog is positive except for the radix-tree stuff, which is nominally kernel-ish ...? Userspace clearly needs some cleanups; mkfs parsers return negative, everything else seems positive; a couple metadump functions return negative, the rest is positive; most userspace callers of libxfs negate the negative error return... So yeah I'm of the opinion that unless it's kernel(-ish?) code it should be positive, and I can send a patch to clean up stuff that's not. I can be swayed by counterarguments if you have them. :) -Eric
On Tue, May 21, 2019 at 01:59:58PM -0500, Eric Sandeen wrote: > So yeah I'm of the opinion that unless it's kernel(-ish?) code it should be > positive, and I can send a patch to clean up stuff that's not. > > I can be swayed by counterarguments if you have them. :) What speaks against everything is negative? It isn't like returning positive errors really is a traditional userspace convention, as that is return -1 (negative!) and look at errno..
On 5/21/19 2:19 PM, Christoph Hellwig wrote: > On Tue, May 21, 2019 at 01:59:58PM -0500, Eric Sandeen wrote: >> So yeah I'm of the opinion that unless it's kernel(-ish?) code it should be >> positive, and I can send a patch to clean up stuff that's not. >> >> I can be swayed by counterarguments if you have them. :) > > What speaks against everything is negative? It isn't like returning > positive errors really is a traditional userspace convention, as that > is return -1 (negative!) and look at errno.. Sorry, I wasn't clear - I meant returning negative errnos. That's the part that's not consistent. -Eric
On Tue, May 21, 2019 at 02:20:51PM -0500, Eric Sandeen wrote: > > On Tue, May 21, 2019 at 01:59:58PM -0500, Eric Sandeen wrote: > >> So yeah I'm of the opinion that unless it's kernel(-ish?) code it should be > >> positive, and I can send a patch to clean up stuff that's not. > >> > >> I can be swayed by counterarguments if you have them. :) > > > > What speaks against everything is negative? It isn't like returning > > positive errors really is a traditional userspace convention, as that > > is return -1 (negative!) and look at errno.. > > Sorry, I wasn't clear - I meant returning negative errnos. That's > the part that's not consistent. Yes. And for libxfs/libfrog/etc stuff I think sticking to that and always returning negative error values sounds sanest to me. Note that some userspace libraries have adopted that calling convention, for example libaio.
On 5/21/19 2:28 PM, Christoph Hellwig wrote: > On Tue, May 21, 2019 at 02:20:51PM -0500, Eric Sandeen wrote: >>> On Tue, May 21, 2019 at 01:59:58PM -0500, Eric Sandeen wrote: >>>> So yeah I'm of the opinion that unless it's kernel(-ish?) code it should be >>>> positive, and I can send a patch to clean up stuff that's not. >>>> >>>> I can be swayed by counterarguments if you have them. :) >>> >>> What speaks against everything is negative? It isn't like returning >>> positive errors really is a traditional userspace convention, as that >>> is return -1 (negative!) and look at errno.. >> >> Sorry, I wasn't clear - I meant returning negative errnos. That's >> the part that's not consistent. > > Yes. And for libxfs/libfrog/etc stuff I think sticking to that and > always returning negative error values sounds sanest to me. Note that > some userspace libraries have adopted that calling convention, for > example libaio. > Oh, I see what you mean. *shrug* sure, that makes sense. Sorry, it's my weird old SGI/Irix/ancient-xfs history confusing me. ;) -Eric
On 5/20/19 6:17 PM, Darrick J. Wong wrote: > From: Darrick J. Wong <darrick.wong@oracle.com> > > Fix the return types of non-predicate bitmap functions to return the > usual negative error codes instead of the "moveon" boolean. > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com> > --- > include/bitmap.h | 8 +++-- > libfrog/bitmap.c | 86 ++++++++++++++++++++++++++---------------------------- > repair/rmap.c | 18 ++++++++--- > scrub/phase6.c | 18 ++++------- > 4 files changed, 65 insertions(+), 65 deletions(-) Sorry for the drama on signs. Reviewed-by: Eric Sandeen <sandeen@redhat.com> > > diff --git a/include/bitmap.h b/include/bitmap.h > index e29a4335..99a2fb23 100644 > --- a/include/bitmap.h > +++ b/include/bitmap.h > @@ -11,11 +11,11 @@ struct bitmap { > struct avl64tree_desc *bt_tree; > }; > > -bool bitmap_init(struct bitmap **bmap); > +int bitmap_init(struct bitmap **bmap); > void bitmap_free(struct bitmap **bmap); > -bool bitmap_set(struct bitmap *bmap, uint64_t start, uint64_t length); > -bool bitmap_iterate(struct bitmap *bmap, > - bool (*fn)(uint64_t, uint64_t, void *), void *arg); > +int bitmap_set(struct bitmap *bmap, uint64_t start, uint64_t length); > +int bitmap_iterate(struct bitmap *bmap, int (*fn)(uint64_t, uint64_t, void *), > + void *arg); > bool bitmap_test(struct bitmap *bmap, uint64_t start, > uint64_t len); > bool bitmap_empty(struct bitmap *bmap); > diff --git a/libfrog/bitmap.c b/libfrog/bitmap.c > index 450ebe0a..4dafc4c9 100644 > --- a/libfrog/bitmap.c > +++ b/libfrog/bitmap.c > @@ -66,7 +66,7 @@ static struct avl64ops bitmap_ops = { > }; > > /* Initialize a bitmap. */ > -bool > +int > bitmap_init( > struct bitmap **bmapp) > { > @@ -74,18 +74,18 @@ bitmap_init( > > bmap = calloc(1, sizeof(struct bitmap)); > if (!bmap) > - return false; > + return -ENOMEM; > bmap->bt_tree = malloc(sizeof(struct avl64tree_desc)); > if (!bmap->bt_tree) { > free(bmap); > - return false; > + return -ENOMEM; > } > > pthread_mutex_init(&bmap->bt_lock, NULL); > avl64_init_tree(bmap->bt_tree, &bitmap_ops); > *bmapp = bmap; > > - return true; > + return 0; > } > > /* Free a bitmap. */ > @@ -127,8 +127,31 @@ bitmap_node_init( > return ext; > } > > +/* Create a new bitmap node and insert it. */ > +static inline int > +__bitmap_insert( > + struct bitmap *bmap, > + uint64_t start, > + uint64_t length) > +{ > + struct bitmap_node *ext; > + struct avl64node *node; > + > + ext = bitmap_node_init(start, length); > + if (!ext) > + return -ENOMEM; > + > + node = avl64_insert(bmap->bt_tree, &ext->btn_node); > + if (node == NULL) { > + free(ext); > + return -EEXIST; > + } > + > + return 0; > +} > + > /* Set a region of bits (locked). */ > -static bool > +static int > __bitmap_set( > struct bitmap *bmap, > uint64_t start, > @@ -142,28 +165,14 @@ __bitmap_set( > struct bitmap_node *ext; > uint64_t new_start; > uint64_t new_length; > - struct avl64node *node; > - bool res = true; > > /* Find any existing nodes adjacent or within that range. */ > avl64_findranges(bmap->bt_tree, start - 1, start + length + 1, > &firstn, &lastn); > > /* Nothing, just insert a new extent. */ > - if (firstn == NULL && lastn == NULL) { > - ext = bitmap_node_init(start, length); > - if (!ext) > - return false; > - > - node = avl64_insert(bmap->bt_tree, &ext->btn_node); > - if (node == NULL) { > - free(ext); > - errno = EEXIST; > - return false; > - } > - > - return true; > - } > + if (firstn == NULL && lastn == NULL) > + return __bitmap_insert(bmap, start, length); > > assert(firstn != NULL && lastn != NULL); > new_start = start; > @@ -175,7 +184,7 @@ __bitmap_set( > /* Bail if the new extent is contained within an old one. */ > if (ext->btn_start <= start && > ext->btn_start + ext->btn_length >= start + length) > - return res; > + return 0; > > /* Check for overlapping and adjacent extents. */ > if (ext->btn_start + ext->btn_length >= start || > @@ -195,28 +204,17 @@ __bitmap_set( > } > } > > - ext = bitmap_node_init(new_start, new_length); > - if (!ext) > - return false; > - > - node = avl64_insert(bmap->bt_tree, &ext->btn_node); > - if (node == NULL) { > - free(ext); > - errno = EEXIST; > - return false; > - } > - > - return res; > + return __bitmap_insert(bmap, new_start, new_length); > } > > /* Set a region of bits. */ > -bool > +int > bitmap_set( > struct bitmap *bmap, > uint64_t start, > uint64_t length) > { > - bool res; > + int res; > > pthread_mutex_lock(&bmap->bt_lock); > res = __bitmap_set(bmap, start, length); > @@ -308,26 +306,26 @@ bitmap_clear( > > #ifdef DEBUG > /* Iterate the set regions of this bitmap. */ > -bool > +int > bitmap_iterate( > struct bitmap *bmap, > - bool (*fn)(uint64_t, uint64_t, void *), > + int (*fn)(uint64_t, uint64_t, void *), > void *arg) > { > struct avl64node *node; > struct bitmap_node *ext; > - bool moveon = true; > + int error = 0; > > pthread_mutex_lock(&bmap->bt_lock); > avl_for_each(bmap->bt_tree, node) { > ext = container_of(node, struct bitmap_node, btn_node); > - moveon = fn(ext->btn_start, ext->btn_length, arg); > - if (!moveon) > + error = fn(ext->btn_start, ext->btn_length, arg); > + if (error) > break; > } > pthread_mutex_unlock(&bmap->bt_lock); > > - return moveon; > + return error; > } > #endif > > @@ -372,14 +370,14 @@ bitmap_empty( > } > > #ifdef DEBUG > -static bool > +static int > bitmap_dump_fn( > uint64_t startblock, > uint64_t blockcount, > void *arg) > { > printf("%"PRIu64":%"PRIu64"\n", startblock, blockcount); > - return true; > + return 0; > } > > /* Dump bitmap. */ > diff --git a/repair/rmap.c b/repair/rmap.c > index 19cceca3..47828a06 100644 > --- a/repair/rmap.c > +++ b/repair/rmap.c > @@ -490,16 +490,22 @@ rmap_store_ag_btree_rec( > error = init_slab_cursor(ag_rmap->ar_raw_rmaps, rmap_compare, &rm_cur); > if (error) > goto err; > - if (!bitmap_init(&own_ag_bitmap)) { > - error = -ENOMEM; > + error = -bitmap_init(&own_ag_bitmap); > + if (error) > goto err_slab; > - } > while ((rm_rec = pop_slab_cursor(rm_cur)) != NULL) { > if (rm_rec->rm_owner != XFS_RMAP_OWN_AG) > continue; > - if (!bitmap_set(own_ag_bitmap, rm_rec->rm_startblock, > - rm_rec->rm_blockcount)) { > - error = EFSCORRUPTED; > + error = -bitmap_set(own_ag_bitmap, rm_rec->rm_startblock, > + rm_rec->rm_blockcount); > + if (error) { > + /* > + * If this range is already set, then the incore rmap > + * records for the AG free space btrees overlap and > + * we're toast because that is not allowed. > + */ > + if (error == EEXIST) > + error = EFSCORRUPTED; > goto err_slab; > } > } > diff --git a/scrub/phase6.c b/scrub/phase6.c > index 4b25f3bb..66e6451c 100644 > --- a/scrub/phase6.c > +++ b/scrub/phase6.c > @@ -341,7 +341,6 @@ xfs_check_rmap_ioerr( > struct media_verify_state *vs = arg; > struct bitmap *tree; > dev_t dev; > - bool moveon; > > dev = xfs_disk_to_dev(ctx, disk); > > @@ -356,8 +355,8 @@ xfs_check_rmap_ioerr( > else > tree = NULL; > if (tree) { > - moveon = bitmap_set(tree, start, length); > - if (!moveon) > + errno = -bitmap_set(tree, start, length); > + if (errno) > str_errno(ctx, ctx->mntpoint); > } > > @@ -454,16 +453,16 @@ xfs_scan_blocks( > struct scrub_ctx *ctx) > { > struct media_verify_state vs = { NULL }; > - bool moveon; > + bool moveon = false; > > - moveon = bitmap_init(&vs.d_bad); > - if (!moveon) { > + errno = -bitmap_init(&vs.d_bad); > + if (errno) { > str_errno(ctx, ctx->mntpoint); > goto out; > } > > - moveon = bitmap_init(&vs.r_bad); > - if (!moveon) { > + errno = -bitmap_init(&vs.r_bad); > + if (errno) { > str_errno(ctx, ctx->mntpoint); > goto out_dbad; > } > @@ -472,7 +471,6 @@ xfs_scan_blocks( > ctx->geo.blocksize, xfs_check_rmap_ioerr, > scrub_nproc(ctx)); > if (!vs.rvp_data) { > - moveon = false; > str_info(ctx, ctx->mntpoint, > _("Could not create data device media verifier.")); > goto out_rbad; > @@ -482,7 +480,6 @@ _("Could not create data device media verifier.")); > ctx->geo.blocksize, xfs_check_rmap_ioerr, > scrub_nproc(ctx)); > if (!vs.rvp_log) { > - moveon = false; > str_info(ctx, ctx->mntpoint, > _("Could not create log device media verifier.")); > goto out_datapool; > @@ -493,7 +490,6 @@ _("Could not create data device media verifier.")); > ctx->geo.blocksize, xfs_check_rmap_ioerr, > scrub_nproc(ctx)); > if (!vs.rvp_realtime) { > - moveon = false; > str_info(ctx, ctx->mntpoint, > _("Could not create realtime device media verifier.")); > goto out_logpool; >
diff --git a/include/bitmap.h b/include/bitmap.h index e29a4335..99a2fb23 100644 --- a/include/bitmap.h +++ b/include/bitmap.h @@ -11,11 +11,11 @@ struct bitmap { struct avl64tree_desc *bt_tree; }; -bool bitmap_init(struct bitmap **bmap); +int bitmap_init(struct bitmap **bmap); void bitmap_free(struct bitmap **bmap); -bool bitmap_set(struct bitmap *bmap, uint64_t start, uint64_t length); -bool bitmap_iterate(struct bitmap *bmap, - bool (*fn)(uint64_t, uint64_t, void *), void *arg); +int bitmap_set(struct bitmap *bmap, uint64_t start, uint64_t length); +int bitmap_iterate(struct bitmap *bmap, int (*fn)(uint64_t, uint64_t, void *), + void *arg); bool bitmap_test(struct bitmap *bmap, uint64_t start, uint64_t len); bool bitmap_empty(struct bitmap *bmap); diff --git a/libfrog/bitmap.c b/libfrog/bitmap.c index 450ebe0a..4dafc4c9 100644 --- a/libfrog/bitmap.c +++ b/libfrog/bitmap.c @@ -66,7 +66,7 @@ static struct avl64ops bitmap_ops = { }; /* Initialize a bitmap. */ -bool +int bitmap_init( struct bitmap **bmapp) { @@ -74,18 +74,18 @@ bitmap_init( bmap = calloc(1, sizeof(struct bitmap)); if (!bmap) - return false; + return -ENOMEM; bmap->bt_tree = malloc(sizeof(struct avl64tree_desc)); if (!bmap->bt_tree) { free(bmap); - return false; + return -ENOMEM; } pthread_mutex_init(&bmap->bt_lock, NULL); avl64_init_tree(bmap->bt_tree, &bitmap_ops); *bmapp = bmap; - return true; + return 0; } /* Free a bitmap. */ @@ -127,8 +127,31 @@ bitmap_node_init( return ext; } +/* Create a new bitmap node and insert it. */ +static inline int +__bitmap_insert( + struct bitmap *bmap, + uint64_t start, + uint64_t length) +{ + struct bitmap_node *ext; + struct avl64node *node; + + ext = bitmap_node_init(start, length); + if (!ext) + return -ENOMEM; + + node = avl64_insert(bmap->bt_tree, &ext->btn_node); + if (node == NULL) { + free(ext); + return -EEXIST; + } + + return 0; +} + /* Set a region of bits (locked). */ -static bool +static int __bitmap_set( struct bitmap *bmap, uint64_t start, @@ -142,28 +165,14 @@ __bitmap_set( struct bitmap_node *ext; uint64_t new_start; uint64_t new_length; - struct avl64node *node; - bool res = true; /* Find any existing nodes adjacent or within that range. */ avl64_findranges(bmap->bt_tree, start - 1, start + length + 1, &firstn, &lastn); /* Nothing, just insert a new extent. */ - if (firstn == NULL && lastn == NULL) { - ext = bitmap_node_init(start, length); - if (!ext) - return false; - - node = avl64_insert(bmap->bt_tree, &ext->btn_node); - if (node == NULL) { - free(ext); - errno = EEXIST; - return false; - } - - return true; - } + if (firstn == NULL && lastn == NULL) + return __bitmap_insert(bmap, start, length); assert(firstn != NULL && lastn != NULL); new_start = start; @@ -175,7 +184,7 @@ __bitmap_set( /* Bail if the new extent is contained within an old one. */ if (ext->btn_start <= start && ext->btn_start + ext->btn_length >= start + length) - return res; + return 0; /* Check for overlapping and adjacent extents. */ if (ext->btn_start + ext->btn_length >= start || @@ -195,28 +204,17 @@ __bitmap_set( } } - ext = bitmap_node_init(new_start, new_length); - if (!ext) - return false; - - node = avl64_insert(bmap->bt_tree, &ext->btn_node); - if (node == NULL) { - free(ext); - errno = EEXIST; - return false; - } - - return res; + return __bitmap_insert(bmap, new_start, new_length); } /* Set a region of bits. */ -bool +int bitmap_set( struct bitmap *bmap, uint64_t start, uint64_t length) { - bool res; + int res; pthread_mutex_lock(&bmap->bt_lock); res = __bitmap_set(bmap, start, length); @@ -308,26 +306,26 @@ bitmap_clear( #ifdef DEBUG /* Iterate the set regions of this bitmap. */ -bool +int bitmap_iterate( struct bitmap *bmap, - bool (*fn)(uint64_t, uint64_t, void *), + int (*fn)(uint64_t, uint64_t, void *), void *arg) { struct avl64node *node; struct bitmap_node *ext; - bool moveon = true; + int error = 0; pthread_mutex_lock(&bmap->bt_lock); avl_for_each(bmap->bt_tree, node) { ext = container_of(node, struct bitmap_node, btn_node); - moveon = fn(ext->btn_start, ext->btn_length, arg); - if (!moveon) + error = fn(ext->btn_start, ext->btn_length, arg); + if (error) break; } pthread_mutex_unlock(&bmap->bt_lock); - return moveon; + return error; } #endif @@ -372,14 +370,14 @@ bitmap_empty( } #ifdef DEBUG -static bool +static int bitmap_dump_fn( uint64_t startblock, uint64_t blockcount, void *arg) { printf("%"PRIu64":%"PRIu64"\n", startblock, blockcount); - return true; + return 0; } /* Dump bitmap. */ diff --git a/repair/rmap.c b/repair/rmap.c index 19cceca3..47828a06 100644 --- a/repair/rmap.c +++ b/repair/rmap.c @@ -490,16 +490,22 @@ rmap_store_ag_btree_rec( error = init_slab_cursor(ag_rmap->ar_raw_rmaps, rmap_compare, &rm_cur); if (error) goto err; - if (!bitmap_init(&own_ag_bitmap)) { - error = -ENOMEM; + error = -bitmap_init(&own_ag_bitmap); + if (error) goto err_slab; - } while ((rm_rec = pop_slab_cursor(rm_cur)) != NULL) { if (rm_rec->rm_owner != XFS_RMAP_OWN_AG) continue; - if (!bitmap_set(own_ag_bitmap, rm_rec->rm_startblock, - rm_rec->rm_blockcount)) { - error = EFSCORRUPTED; + error = -bitmap_set(own_ag_bitmap, rm_rec->rm_startblock, + rm_rec->rm_blockcount); + if (error) { + /* + * If this range is already set, then the incore rmap + * records for the AG free space btrees overlap and + * we're toast because that is not allowed. + */ + if (error == EEXIST) + error = EFSCORRUPTED; goto err_slab; } } diff --git a/scrub/phase6.c b/scrub/phase6.c index 4b25f3bb..66e6451c 100644 --- a/scrub/phase6.c +++ b/scrub/phase6.c @@ -341,7 +341,6 @@ xfs_check_rmap_ioerr( struct media_verify_state *vs = arg; struct bitmap *tree; dev_t dev; - bool moveon; dev = xfs_disk_to_dev(ctx, disk); @@ -356,8 +355,8 @@ xfs_check_rmap_ioerr( else tree = NULL; if (tree) { - moveon = bitmap_set(tree, start, length); - if (!moveon) + errno = -bitmap_set(tree, start, length); + if (errno) str_errno(ctx, ctx->mntpoint); } @@ -454,16 +453,16 @@ xfs_scan_blocks( struct scrub_ctx *ctx) { struct media_verify_state vs = { NULL }; - bool moveon; + bool moveon = false; - moveon = bitmap_init(&vs.d_bad); - if (!moveon) { + errno = -bitmap_init(&vs.d_bad); + if (errno) { str_errno(ctx, ctx->mntpoint); goto out; } - moveon = bitmap_init(&vs.r_bad); - if (!moveon) { + errno = -bitmap_init(&vs.r_bad); + if (errno) { str_errno(ctx, ctx->mntpoint); goto out_dbad; } @@ -472,7 +471,6 @@ xfs_scan_blocks( ctx->geo.blocksize, xfs_check_rmap_ioerr, scrub_nproc(ctx)); if (!vs.rvp_data) { - moveon = false; str_info(ctx, ctx->mntpoint, _("Could not create data device media verifier.")); goto out_rbad; @@ -482,7 +480,6 @@ _("Could not create data device media verifier.")); ctx->geo.blocksize, xfs_check_rmap_ioerr, scrub_nproc(ctx)); if (!vs.rvp_log) { - moveon = false; str_info(ctx, ctx->mntpoint, _("Could not create log device media verifier.")); goto out_datapool; @@ -493,7 +490,6 @@ _("Could not create data device media verifier.")); ctx->geo.blocksize, xfs_check_rmap_ioerr, scrub_nproc(ctx)); if (!vs.rvp_realtime) { - moveon = false; str_info(ctx, ctx->mntpoint, _("Could not create realtime device media verifier.")); goto out_logpool;