Add libxo support to du #1145

nhuff · 2024-03-24T15:31:02Z

Convert du to use libxo enabling structured output.

Signed-off-by: Nathan Huff nhuff@acm.org

bsdimp · 2024-04-12T17:06:28Z

usr.bin/du/du.c


 #define SI_OPT	(CHAR_MAX + 1)

-#define UNITS_2		1
+#define UNITS_2	1


This seems gratuitous.

bsdimp · 2024-04-12T17:06:49Z

usr.bin/du/du.c

@@ -62,7 +63,7 @@ struct ignentry {

 static int	linkchk(FTSENT *);
 static void	usage(void);
-static void	prthumanval(int64_t);
+static void	prthumanval(const char*, int64_t);


FreeBSD's style is const char * with a space after the char.

bsdimp · 2024-04-12T17:07:46Z

usr.bin/du/du.c

@@ -273,15 +280,17 @@ main(int argc, char *argv[])
 			if (p->fts_level <= depth && threshold <=
 			    threshold_sign * howmany(p->fts_bignum *
 			    cblocksize, blocksize)) {
+				xo_open_instance("paths");


unless I'm missing something, and maybe I am, why are we opening this twice? Here and at line 263?

bsdimp · 2024-04-12T17:08:22Z

usr.bin/du/du.c

@@ -309,36 +318,40 @@ main(int argc, char *argv[])
 			    howmany(p->fts_statp->st_blocks, cblocksize);

 			if (aflag || p->fts_level == 0) {
+				xo_open_instance("paths");
 				if (hflag > 0) {


Same question.

bsdimp · 2024-04-12T17:09:12Z

usr.bin/du/du.c

 		/* Malloc one if we have to. */
 		le = malloc(sizeof(struct links_entry));
+	}


This is a good change, but it's gratuitous since you're not changing adjacent lines.

bsdimp

two minor nits that I see, and either a double push of paths, or something that I don't understand.

nhuff · 2024-04-12T18:19:50Z

I've updated the pull request to address the style issues.

The two xo_open_instance("paths") calls happen in two different code paths during the filesystem traversal. One path is taken if you are reporting on a file and one path if you are reporting on a directory.

markjdb

Sorry, I had left these comments last week, but apparently failed to click submit. :(

markjdb · 2024-04-05T18:04:05Z

usr.bin/du/du.c

@@ -107,6 +108,10 @@ main(int argc, char *argv[])
 	depth = INT_MAX;
 	SLIST_INIT(&ignores);

+	argc = xo_parse_args(argc, argv);
+	if (argc < 0)
+		exit(-1);


It'd be more consistent to use EX_USAGE like usage() does.

Do you have an in tree example of what you would like to see here? I'm trying to be consistent and all of the examples I'm finding just exit in this case.

I meant, this line should be exit(EX_USAGE). It's unusual to exit with status -1.

markjdb · 2024-04-05T18:04:44Z

usr.bin/du/du.c

 		/* Malloc one if we have to. */
 		le = malloc(sizeof(struct links_entry));
+	}
 	if (le == NULL) {
 		stop_allocating = 1;
 		warnx("No more memory for tracking hard links");


Looks like some warnx() calls were not converted?

I'm using df as a template. It didn't look like they converted the warnx on malloc failures. Does xo_warnx allocate? I figured it might and that is why df does it the way it does, but maybe I should check for sure.

By default I believe that xo_warnx() etc. just output unstructured data to standard error, and they don't appear to allocate memory. (And libxo will print an error if it hits an internal memory allocation failure.)

In tree examples seem somewhat inconsistent here so I don't have any issues converting the rest of the calls.

markjdb · 2024-04-05T18:07:58Z

usr.bin/du/du.c

 			    savednumber * cblocksize, blocksize));
 		}
 	}

 	ignoreclean();
+	xo_close_container("disk-usage-information");
+	xo_finish();


We're not very consistent about it in the tree yet, but we should check for errors from xo_finish():

if (xo_finish() < 0) xo_err(1, "stdout");

markjdb · 2024-04-05T18:09:54Z

usr.bin/du/du.c

@@ -488,13 +502,13 @@ prthumanval(int64_t bytes)

 	humanize_number(buf, sizeof(buf), bytes, "", HN_AUTOSCALE, flags);

-	(void)printf("%4s", buf);


The width of the string here is coupled with the size of buf, so this change isn't quite right. Should we instead have the caller pass a buffer to prthumanval()?

I don't think this PR changes the behavior here. It changes from printf("%4s", buf) to an xo_emit with a format string that also always uses "%4s". There might be an issue here, but if so I think it is pre-existing.

Convert du to use libxo enabling structured output. Signed-off-by: Nathan Huff <nhuff@acm.org>

nhuff · 2024-04-12T20:45:53Z

I've updated the pull request to address most of the comments from @markjdb. I didn't address the last comment since I don't think the behavior differs from the current implementation in any relevant way.

bsdimp · 2024-04-23T22:53:09Z

my eyeballs say that this is ready. Will run some tests and either give feedback or push.

bsdimp · 2024-04-29T05:02:18Z

usr.bin/du/du.c

 			    savednumber * cblocksize, blocksize));
 		}
 	}

 	ignoreclean();
+	xo_close_container("disk-usage-information");
+	if(xo_finish() <0)


Not sure why this wasn't flagged with the style checker. if space ( and there needs to be a space after <.

bsdimp · 2024-04-29T05:03:20Z

usr.bin/du/du.c

 				if (hflag > 0) {
-					prthumanval(p->fts_bignum);
-					(void)printf("\t%s\n", p->fts_path);
+					prthumanval("{:blocks/%4s}",p->fts_bignum);


this needs a space after the comma.

bsdimp · 2024-04-29T05:03:53Z

usr.bin/du/du.c


 	if (cflag) {
 		if (hflag > 0) {
-			prthumanval(savednumber);
-			(void)printf("\ttotal\n");
+			prthumanval("{:total-blocks/%4s}\ttotal\n",savednumber);


this also needs a space before saved number.

bsdimp

Please fix the style issues tagged.

bsdimp · 2024-04-29T05:09:11Z

this looks like it will work... and spot checking shows that it does. Just a couple style issue to fix.

Convert du to use libxo enabling structured output. [[ minor style fixes by imp ]] Signed-off-by: Nathan Huff <nhuff@acm.org> Reviewed by: imp Pull Request: #1145

bsdimp · 2024-04-29T14:26:55Z

On second thought, the style issues were relatively minor and easy to fix while staging.
I also bumped the date on the man page, but that's OK: many man pages are active enough that dates conflict a lot.
I went ahead and fixed them this time, but next time please use checkstyle9.pl to make future submissions better.
Thanks for this new functionality.

bsdimp reviewed Apr 12, 2024

View reviewed changes

usr.bin/du/du.c Outdated

#define SI_OPT (CHAR_MAX + 1)

#define UNITS_2 1

#define UNITS_2 1

Copy link

Member

bsdimp Apr 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems gratuitous.

bsdimp reviewed Apr 12, 2024

View reviewed changes

bsdimp requested changes Apr 12, 2024

View reviewed changes

bsdimp added the changes-required label Apr 12, 2024

nhuff force-pushed the du-ox branch from 281ba48 to 2914d5a Compare April 12, 2024 18:15

markjdb reviewed Apr 12, 2024

View reviewed changes

Add libxo support to du

5624b11

Convert du to use libxo enabling structured output. Signed-off-by: Nathan Huff <nhuff@acm.org>

nhuff force-pushed the du-ox branch from 2914d5a to 5624b11 Compare April 12, 2024 20:43

bsdimp self-assigned this Apr 19, 2024

bsdimp added ready needs-kyua-run and removed changes-required labels Apr 23, 2024

bsdimp reviewed Apr 29, 2024

View reviewed changes

bsdimp requested changes Apr 29, 2024

View reviewed changes

bsdimp added changes-required and removed ready needs-kyua-run labels Apr 29, 2024

bsdimp added merged and removed changes-required labels Apr 29, 2024

freebsd-git pushed a commit that referenced this pull request Apr 29, 2024

Add libxo support to du

b2b8fa1

Convert du to use libxo enabling structured output. [[ minor style fixes by imp ]] Signed-off-by: Nathan Huff <nhuff@acm.org> Reviewed by: imp Pull Request: #1145

bsdimp closed this Apr 29, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add libxo support to du #1145

Add libxo support to du #1145

nhuff commented Mar 24, 2024

bsdimp Apr 12, 2024

bsdimp Apr 12, 2024

bsdimp Apr 12, 2024

bsdimp Apr 12, 2024

bsdimp Apr 12, 2024

bsdimp left a comment

nhuff commented Apr 12, 2024

markjdb left a comment

markjdb Apr 5, 2024

nhuff Apr 12, 2024

markjdb Apr 12, 2024 •

edited

markjdb Apr 5, 2024

nhuff Apr 12, 2024

markjdb Apr 12, 2024

nhuff Apr 12, 2024

markjdb Apr 5, 2024

markjdb Apr 5, 2024

nhuff Apr 12, 2024

nhuff commented Apr 12, 2024

bsdimp commented Apr 23, 2024

bsdimp Apr 29, 2024

bsdimp Apr 29, 2024

bsdimp Apr 29, 2024

bsdimp left a comment

bsdimp commented Apr 29, 2024

bsdimp commented Apr 29, 2024

		@@ -488,13 +502,13 @@ prthumanval(int64_t bytes)

		humanize_number(buf, sizeof(buf), bytes, "", HN_AUTOSCALE, flags);

		(void)printf("%4s", buf);

Add libxo support to du #1145

Add libxo support to du #1145

Conversation

nhuff commented Mar 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bsdimp left a comment

Choose a reason for hiding this comment

nhuff commented Apr 12, 2024

markjdb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markjdb Apr 12, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nhuff commented Apr 12, 2024

bsdimp commented Apr 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bsdimp left a comment

Choose a reason for hiding this comment

bsdimp commented Apr 29, 2024

bsdimp commented Apr 29, 2024

markjdb Apr 12, 2024 •

edited