Page MenuHomeFreeBSD

mount(8): Add libxo(3) support
ClosedPublic

Authored by me_cameronkatri.com on May 18 2021, 9:09 PM.

Details

Reviewers
None
Group Reviewers
manpages
Commits
rGe725ee7eb672: mount: add libxo(3) support
Test Plan
  • Text output is the same.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

me_cameronkatri.com created this revision.

Use no quote modifier on dump and pass.

otis added inline comments.
sbin/mount/mount.c
155

Aren't these lines (151, 152) a suitable candidate for a macro?

me_cameronkatri.com edited the test plan for this revision. (Show Details)

Open the mount container before any calls to xo_errx
Use a macro to close the container, finish and exit.

I think the xo_emit() stuff is right, but I'm not sure.
The rest of the stuff seems good, modulo my comments.

sbin/mount/mount.c
68

Minor nit: I'd align all the \ if they aren't already (it's hard to tell with phab sometimes, but I think there's a tab missing)

256

This doesn't make any sense. Negative return values from main are weird.
Better to do a xo_errx(1, "Can't parse arges") (or w/o the xo_ prefix if this isn't allowed). Return '1' if there's already an error message printed so it isn't doubled.

674

this is indented too far. It should just be tab 4-spaces.

Revert last diff (uploaded to wrong differential)

me_cameronkatri.com added inline comments.
sbin/mount/mount.c
68

This looks aligned in vim with freebsd.vim.

pstef added inline comments.
sbin/mount/mount.c
710

I'm not sure I understand the purpose of this, as asprintf will allocate the buffer and set fsidbuf to point to it.

714–718

Probably should check the return value of asprintf.

914

Should we update usage()?

me_cameronkatri.com marked an inline comment as done.

Check the return value of asprintf.

me_cameronkatri.com added inline comments.
sbin/mount/mount.c
710

If I don't do this, when using --libxo json the fsid will start with (null), I'm sure there is a better way to stop that but this worked.

914

Should we update usage()?

It doesn't look like any other commands show --libxo in their usage.

sbin/mount/mount.c
710

There are some 32-bit ints in fsid_t and we want to interpret them as unsigned bytes and print using hexadecimal representation. So it's a matter of calculating the needed space once and using that space in a loop, like this:

		if (sfp->f_fsid.val[0] != 0 || sfp->f_fsid.val[1] != 0) {
			xo_emit("{D:, }{Lw:fsid}");
			fsidbuf = malloc(sizeof(sfp->f_fsid) * 2 + 1);
			if (fsidbuf == NULL)
				xo_errx(1, "malloc failed");
			for (i = 0; i < sizeof(sfp->f_fsid); i++)
				sprintf(&fsidbuf[i * 2], "%02x",
				    ((u_char *)&sfp->f_fsid)[i]);
			fsidbuf[i * 2] = '\0';
			xo_emit("{:fsid}", fsidbuf);
			free(fsidbuf);
		}
sbin/mount/mount.c
710

And then it seems the two xo_emit() calls can be folded into xo_emit("{D:, }{Lw:fsid}{:fsid}", fsidbuf);

me_cameronkatri.com retitled this revision from Add libxo(3) support to mount(8) to mount(8): Add libxo(3) support.Sep 24 2021, 7:59 PM
me_cameronkatri.com edited the summary of this revision. (Show Details)

Change fsid allocation. Thanks pstef.

This revision was not accepted when it landed; it landed in state Needs Review.Sep 24 2021, 9:10 PM
Closed by commit rGe725ee7eb672: mount: add libxo(3) support (authored by me_cameronkatri.com, committed by pstef). · Explain Why
This revision was automatically updated to reflect the committed changes.

xo_parse_args gives error messages for invalid arguments; no further error is needed.

https://libxo.readthedocs.io/en/latest/api.html?#parsing-command-line-arguments-xo-parse-args

argc = xo_parse_args(argc, argv);
if (argc < 0)
    exit(EXIT_FAILURE);

Also looks like you've lost the space in "write: sync". Use "{Lwc:writes}{P: }{Lw:sync}".

Could you please give me more details about your comment "If I don't do this, when using --libxo json the fsid will start with (null),", since we don't want that to be happening?

Thanks,
Phil

One more thing: Does libxo need an xo_exit() function? It would be just xo_finish + exit, like your EXIT macro, but might help minimize diffs like this.

Thanks,
Phil

In D30341#727179, @phil wrote:

Also looks like you've lost the space in "write: sync". Use "{Lwc:writes}{P: }{Lw:sync}".

diff -u <(sudo /usr/obj/usr/src/amd64.amd64/sbin/mount/mount -v) <(sudo mount -v) doesn't show any difference between the outputs before and after this commit.

In D30341#727179, @phil wrote:

Could you please give me more details about your comment "If I don't do this, when using --libxo json the fsid will start with (null),", since we don't want that to be happening?

That was about a previous version of this diff and it had more to do with memory management and strings than with libxo.