diff mbox series

[01/13] libfrog: fix workqueue error communication problems

Message ID 156944720949.297677.6259044122864374968.stgit@magnolia (mailing list archive)
State Accepted
Headers show
Series libfrog/xfs_scrub: fix error handling | expand

Commit Message

Darrick J. Wong Sept. 25, 2019, 9:33 p.m. UTC 
From: Darrick J. Wong <darrick.wong@oracle.com>

Convert all the workqueue functions to return positive error codes so
that we can move away from the libc-style indirect errno handling and
towards passing error codes directly back to callers.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 libfrog/workqueue.c |    4 ++--
 scrub/fscounters.c  |    5 ++---
 scrub/inodes.c      |    5 ++---
 scrub/phase2.c      |    8 +++-----
 scrub/phase4.c      |    6 +++---
 scrub/read_verify.c |    3 +--
 scrub/spacemap.c    |   11 ++++-------
 scrub/vfs.c         |    3 +--
 8 files changed, 18 insertions(+), 27 deletions(-)

Comments

Eric Sandeen Sept. 30, 2019, 7:23 p.m. UTC | #1 
On 9/25/19 4:33 PM, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Convert all the workqueue functions to return positive error codes so
> that we can move away from the libc-style indirect errno handling and
> towards passing error codes directly back to callers.

This all looks fine, but it doesn't really do what the commit log says,
right?  The one spot where error return is changed, it was already
positive; the rest is swapping str_liberror for str_info which is
just cosmetic, right?

-Eric

> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  libfrog/workqueue.c |    4 ++--
>  scrub/fscounters.c  |    5 ++---
>  scrub/inodes.c      |    5 ++---
>  scrub/phase2.c      |    8 +++-----
>  scrub/phase4.c      |    6 +++---
>  scrub/read_verify.c |    3 +--
>  scrub/spacemap.c    |   11 ++++-------
>  scrub/vfs.c         |    3 +--
>  8 files changed, 18 insertions(+), 27 deletions(-)
> 
> 
> diff --git a/libfrog/workqueue.c b/libfrog/workqueue.c
> index 73114773..a806da3e 100644
> --- a/libfrog/workqueue.c
> +++ b/libfrog/workqueue.c
> @@ -106,8 +106,8 @@ workqueue_add(
>  	}
>  
>  	wi = malloc(sizeof(struct workqueue_item));
> -	if (wi == NULL)
> -		return ENOMEM;
> +	if (!wi)
> +		return errno;
>  
>  	wi->function = func;
>  	wi->index = index;
> diff --git a/scrub/fscounters.c b/scrub/fscounters.c
> index ad467e0c..669c5ab0 100644
> --- a/scrub/fscounters.c
> +++ b/scrub/fscounters.c
> @@ -115,15 +115,14 @@ xfs_count_all_inodes(
>  			scrub_nproc_workqueue(ctx));
>  	if (ret) {
>  		moveon = false;
> -		str_info(ctx, ctx->mntpoint, _("Could not create workqueue."));
> +		str_liberror(ctx, ret, _("creating icount workqueue"));
>  		goto out_free;
>  	}
>  	for (agno = 0; agno < ctx->mnt.fsgeom.agcount; agno++) {
>  		ret = workqueue_add(&wq, xfs_count_ag_inodes, agno, ci);
>  		if (ret) {
>  			moveon = false;
> -			str_info(ctx, ctx->mntpoint,
> -_("Could not queue AG %u icount work."), agno);
> +			str_liberror(ctx, ret, _("queueing icount work"));
>  			break;
>  		}
>  	}
...
Darrick J. Wong Sept. 30, 2019, 7:29 p.m. UTC | #2 
On Mon, Sep 30, 2019 at 02:23:40PM -0500, Eric Sandeen wrote:
> On 9/25/19 4:33 PM, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > Convert all the workqueue functions to return positive error codes so
> > that we can move away from the libc-style indirect errno handling and
> > towards passing error codes directly back to callers.
> 
> This all looks fine, but it doesn't really do what the commit log says,
> right?

Urrrk... yes.  Clearly I stamped out the changelogs with a machine. :/

I /think/ most of the patches in that series actually have a return
conversion and a callsite conversion, but this one clearly is just...

"Convert workqueue functions to return errno errors from the C library,
then convert the callers to use str_liberror to report the runtime
error."

> The one spot where error return is changed, it was already
> positive; the rest is swapping str_liberror for str_info which is
> just cosmetic, right?

<shrug> Mostly cosmetic.  Before you'd get:

INFO: Could not create workqueue

Now you get:

ERROR: creating icount workqueue: Not enough frobs.

(and it actually records it as a runtime error :P)

--D

> -Eric
> 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
> >  libfrog/workqueue.c |    4 ++--
> >  scrub/fscounters.c  |    5 ++---
> >  scrub/inodes.c      |    5 ++---
> >  scrub/phase2.c      |    8 +++-----
> >  scrub/phase4.c      |    6 +++---
> >  scrub/read_verify.c |    3 +--
> >  scrub/spacemap.c    |   11 ++++-------
> >  scrub/vfs.c         |    3 +--
> >  8 files changed, 18 insertions(+), 27 deletions(-)
> > 
> > 
> > diff --git a/libfrog/workqueue.c b/libfrog/workqueue.c
> > index 73114773..a806da3e 100644
> > --- a/libfrog/workqueue.c
> > +++ b/libfrog/workqueue.c
> > @@ -106,8 +106,8 @@ workqueue_add(
> >  	}
> >  
> >  	wi = malloc(sizeof(struct workqueue_item));
> > -	if (wi == NULL)
> > -		return ENOMEM;
> > +	if (!wi)
> > +		return errno;
> >  
> >  	wi->function = func;
> >  	wi->index = index;
> > diff --git a/scrub/fscounters.c b/scrub/fscounters.c
> > index ad467e0c..669c5ab0 100644
> > --- a/scrub/fscounters.c
> > +++ b/scrub/fscounters.c
> > @@ -115,15 +115,14 @@ xfs_count_all_inodes(
> >  			scrub_nproc_workqueue(ctx));
> >  	if (ret) {
> >  		moveon = false;
> > -		str_info(ctx, ctx->mntpoint, _("Could not create workqueue."));
> > +		str_liberror(ctx, ret, _("creating icount workqueue"));
> >  		goto out_free;
> >  	}
> >  	for (agno = 0; agno < ctx->mnt.fsgeom.agcount; agno++) {
> >  		ret = workqueue_add(&wq, xfs_count_ag_inodes, agno, ci);
> >  		if (ret) {
> >  			moveon = false;
> > -			str_info(ctx, ctx->mntpoint,
> > -_("Could not queue AG %u icount work."), agno);
> > +			str_liberror(ctx, ret, _("queueing icount work"));
> >  			break;
> >  		}
> >  	}
> ...
> 
>
Eric Sandeen Sept. 30, 2019, 7:35 p.m. UTC | #3 
On 9/30/19 2:29 PM, Darrick J. Wong wrote:
> On Mon, Sep 30, 2019 at 02:23:40PM -0500, Eric Sandeen wrote:
>> On 9/25/19 4:33 PM, Darrick J. Wong wrote:
>>> From: Darrick J. Wong <darrick.wong@oracle.com>
>>>
>>> Convert all the workqueue functions to return positive error codes so
>>> that we can move away from the libc-style indirect errno handling and
>>> towards passing error codes directly back to callers.
>>
>> This all looks fine, but it doesn't really do what the commit log says,
>> right?
> 
> Urrrk... yes.  Clearly I stamped out the changelogs with a machine. :/
> 
> I /think/ most of the patches in that series actually have a return
> conversion and a callsite conversion, but this one clearly is just...
> 
> "Convert workqueue functions to return errno errors from the C library,
> then convert the callers to use str_liberror to report the runtime
> error."
> 
>> The one spot where error return is changed, it was already
>> positive; the rest is swapping str_liberror for str_info which is
>> just cosmetic, right?
> 
> <shrug> Mostly cosmetic.  Before you'd get:
> 
> INFO: Could not create workqueue
> 
> Now you get:
> 
> ERROR: creating icount workqueue: Not enough frobs.
> 
> (and it actually records it as a runtime error :P)

Yup, ok - I'll just edit the changelog for you.

-Eric
Eric Sandeen Sept. 30, 2019, 8:32 p.m. UTC | #4 
On 9/25/19 4:33 PM, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Convert all the workqueue functions to return positive error codes so
> that we can move away from the libc-style indirect errno handling and
> towards passing error codes directly back to callers.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

With a fixed changelog,

Reviewed-by: Eric Sandeen <sandeen@redhat.com>
diff mbox series

Patch

diff --git a/libfrog/workqueue.c b/libfrog/workqueue.c
index 73114773..a806da3e 100644
--- a/libfrog/workqueue.c
+++ b/libfrog/workqueue.c
@@ -106,8 +106,8 @@  workqueue_add(
 	}
 
 	wi = malloc(sizeof(struct workqueue_item));
-	if (wi == NULL)
-		return ENOMEM;
+	if (!wi)
+		return errno;
 
 	wi->function = func;
 	wi->index = index;
diff --git a/scrub/fscounters.c b/scrub/fscounters.c
index ad467e0c..669c5ab0 100644
--- a/scrub/fscounters.c
+++ b/scrub/fscounters.c
@@ -115,15 +115,14 @@  xfs_count_all_inodes(
 			scrub_nproc_workqueue(ctx));
 	if (ret) {
 		moveon = false;
-		str_info(ctx, ctx->mntpoint, _("Could not create workqueue."));
+		str_liberror(ctx, ret, _("creating icount workqueue"));
 		goto out_free;
 	}
 	for (agno = 0; agno < ctx->mnt.fsgeom.agcount; agno++) {
 		ret = workqueue_add(&wq, xfs_count_ag_inodes, agno, ci);
 		if (ret) {
 			moveon = false;
-			str_info(ctx, ctx->mntpoint,
-_("Could not queue AG %u icount work."), agno);
+			str_liberror(ctx, ret, _("queueing icount work"));
 			break;
 		}
 	}
diff --git a/scrub/inodes.c b/scrub/inodes.c
index 4cf8a120..644a6372 100644
--- a/scrub/inodes.c
+++ b/scrub/inodes.c
@@ -243,7 +243,7 @@  xfs_scan_all_inodes(
 	ret = workqueue_create(&wq, (struct xfs_mount *)ctx,
 			scrub_nproc_workqueue(ctx));
 	if (ret) {
-		str_info(ctx, ctx->mntpoint, _("Could not create workqueue."));
+		str_liberror(ctx, ret, _("creating bulkstat workqueue"));
 		return false;
 	}
 
@@ -251,8 +251,7 @@  xfs_scan_all_inodes(
 		ret = workqueue_add(&wq, xfs_scan_ag_inodes, agno, &si);
 		if (ret) {
 			si.moveon = false;
-			str_info(ctx, ctx->mntpoint,
-_("Could not queue AG %u bulkstat work."), agno);
+			str_liberror(ctx, ret, _("queueing bulkstat work"));
 			break;
 		}
 	}
diff --git a/scrub/phase2.c b/scrub/phase2.c
index f064c83d..1d2244a4 100644
--- a/scrub/phase2.c
+++ b/scrub/phase2.c
@@ -124,7 +124,7 @@  xfs_scan_metadata(
 	ret = workqueue_create(&wq, (struct xfs_mount *)ctx,
 			scrub_nproc_workqueue(ctx));
 	if (ret) {
-		str_info(ctx, ctx->mntpoint, _("Could not create workqueue."));
+		str_liberror(ctx, ret, _("creating scrub workqueue"));
 		return false;
 	}
 
@@ -145,8 +145,7 @@  xfs_scan_metadata(
 		ret = workqueue_add(&wq, xfs_scan_ag_metadata, agno, &moveon);
 		if (ret) {
 			moveon = false;
-			str_info(ctx, ctx->mntpoint,
-_("Could not queue AG %u scrub work."), agno);
+			str_liberror(ctx, ret, _("queueing per-AG scrub work"));
 			goto out;
 		}
 	}
@@ -157,8 +156,7 @@  _("Could not queue AG %u scrub work."), agno);
 	ret = workqueue_add(&wq, xfs_scan_fs_metadata, 0, &moveon);
 	if (ret) {
 		moveon = false;
-		str_info(ctx, ctx->mntpoint,
-_("Could not queue filesystem scrub work."));
+		str_liberror(ctx, ret, _("queueing per-FS scrub work"));
 		goto out;
 	}
 
diff --git a/scrub/phase4.c b/scrub/phase4.c
index 25fedc83..903da6d2 100644
--- a/scrub/phase4.c
+++ b/scrub/phase4.c
@@ -74,7 +74,7 @@  xfs_process_action_items(
 	ret = workqueue_create(&wq, (struct xfs_mount *)ctx,
 			scrub_nproc_workqueue(ctx));
 	if (ret) {
-		str_error(ctx, ctx->mntpoint, _("Could not create workqueue."));
+		str_liberror(ctx, ret, _("creating repair workqueue"));
 		return false;
 	}
 	for (agno = 0; agno < ctx->mnt.fsgeom.agcount; agno++) {
@@ -82,8 +82,8 @@  xfs_process_action_items(
 			ret = workqueue_add(&wq, xfs_repair_ag, agno, &moveon);
 			if (ret) {
 				moveon = false;
-				str_error(ctx, ctx->mntpoint,
-_("Could not queue repair work."));
+				str_liberror(ctx, ret,
+						_("queueing repair work"));
 				break;
 			}
 		}
diff --git a/scrub/read_verify.c b/scrub/read_verify.c
index 2152d167..ff4d3572 100644
--- a/scrub/read_verify.c
+++ b/scrub/read_verify.c
@@ -197,8 +197,7 @@  read_verify_queue(
 
 	ret = workqueue_add(&rvp->wq, read_verify, 0, tmp);
 	if (ret) {
-		str_info(rvp->ctx, rvp->ctx->mntpoint,
-_("Could not queue read-verify work."));
+		str_liberror(rvp->ctx, ret, _("queueing read-verify work"));
 		free(tmp);
 		return false;
 	}
diff --git a/scrub/spacemap.c b/scrub/spacemap.c
index a7876478..4258e318 100644
--- a/scrub/spacemap.c
+++ b/scrub/spacemap.c
@@ -200,7 +200,7 @@  xfs_scan_all_spacemaps(
 	ret = workqueue_create(&wq, (struct xfs_mount *)ctx,
 			scrub_nproc_workqueue(ctx));
 	if (ret) {
-		str_info(ctx, ctx->mntpoint, _("Could not create workqueue."));
+		str_liberror(ctx, ret, _("creating fsmap workqueue"));
 		return false;
 	}
 	if (ctx->fsinfo.fs_rt) {
@@ -208,8 +208,7 @@  xfs_scan_all_spacemaps(
 				ctx->mnt.fsgeom.agcount + 1, &sbx);
 		if (ret) {
 			sbx.moveon = false;
-			str_info(ctx, ctx->mntpoint,
-_("Could not queue rtdev fsmap work."));
+			str_liberror(ctx, ret, _("queueing rtdev fsmap work"));
 			goto out;
 		}
 	}
@@ -218,8 +217,7 @@  _("Could not queue rtdev fsmap work."));
 				ctx->mnt.fsgeom.agcount + 2, &sbx);
 		if (ret) {
 			sbx.moveon = false;
-			str_info(ctx, ctx->mntpoint,
-_("Could not queue logdev fsmap work."));
+			str_liberror(ctx, ret, _("queueing logdev fsmap work"));
 			goto out;
 		}
 	}
@@ -227,8 +225,7 @@  _("Could not queue logdev fsmap work."));
 		ret = workqueue_add(&wq, xfs_scan_ag_blocks, agno, &sbx);
 		if (ret) {
 			sbx.moveon = false;
-			str_info(ctx, ctx->mntpoint,
-_("Could not queue AG %u fsmap work."), agno);
+			str_liberror(ctx, ret, _("queueing per-AG fsmap work"));
 			break;
 		}
 	}
diff --git a/scrub/vfs.c b/scrub/vfs.c
index 1a1482dd..0cff2e3f 100644
--- a/scrub/vfs.c
+++ b/scrub/vfs.c
@@ -102,8 +102,7 @@  queue_subdir(
 	error = workqueue_add(wq, scan_fs_dir, 0, new_sftd);
 	if (error) {
 		dec_nr_dirs(sft);
-		str_info(ctx, ctx->mntpoint,
-_("Could not queue subdirectory scan work."));
+		str_liberror(ctx, error, _("queueing directory scan work"));
 		goto out_path;
 	}