Added strerror(3) as a Rust library #76

emma · 2024-03-01T23:16:53-07:00

emma commented

2024-03-01 23:16:53 -07:00

This makes Rust error messages consistent with C error messages.

emma added the

enhancement

label 2024-03-01 23:16:53 -07:00

emma added 5 commits 2024-03-01 23:16:53 -07:00

sterror(3): added library for C-like error messages; Makefile: some efficiency changes e81703c6e1

fop(1): changed to use strerror(3) 8f956d775c

hru(1): changed to use strerror(3) cf1d16f860

rpn(1): make error messages consistent with other tools 3e6dc5cc46

rpn(1): remove erroneous strerror(3) dependency ca01ca4074

emma requested review from trinity 2024-03-01 23:16:59 -07:00

emma added 1 commit 2024-03-01 23:52:27 -07:00

Makefile: better macro assignment 898044cd43

trinity requested changes 2024-03-02 03:45:11 -07:00

trinity left a comment

See notes.

Makefile Outdated

						
				@ -15,3 +15,2 @@

				CC=cc

				RUSTC=rustc

				CC?=cc

trinity commented

2024-03-02 03:27:45 -07:00

I really like these Makefile changes.

emma marked this conversation as resolved

src/strerror.rs Outdated

						
				@ -0,0 +13,4 @@

				extern "C" { fn strerror(errnum: c_int) -> *mut c_char; }

				/* wrapper function for use in Rust */

				pub fn c_error(err: std::io::Error) -> String {

trinity commented

2024-03-02 03:44:09 -07:00

Don't name things after implementation details because implementations change. errormessage or emsg or something would be better. emessage? I don't know.

Don't name things after implementation details because implementations change. `errormessage` or `emsg` or something would be better. `emessage`? I don't know.

emma marked this conversation as resolved

src/strerror.rs Outdated

						
				@ -0,0 +15,4 @@

				/* wrapper function for use in Rust */

				pub fn c_error(err: std::io::Error) -> String {

					/* Get the raw OS error. If it’s None, what the hell is going on‽ */

					let error = err.raw_os_error().unwrap_or_else(|| { panic!() }) as c_int;

trinity commented

2024-03-02 03:38:40 -07:00

For consistency with C conventions error should be errno.

Instead of panicking, set errno to 0. POSIX requires strerror(3p) to have useful function when its argument is 0.

To save work with inevitable localization stuff it may be wise to use strerror_l(3p) instead with the local locale - if we want to support locales.

For consistency with C conventions `error` should be `errno`. Instead of panicking, set errno to 0. POSIX requires [strerror(3p)](https://www.man7.org/linux/man-pages/man3/strerror.3p.html) to have useful function when its argument is 0. To save work with inevitable localization stuff it may be wise to use strerror_l(3p) instead with the local locale - if we want to support locales.

emma commented

2024-03-02 09:51:45 -07:00

For consistency with C conventions error should be errno.

This is not C and err is not a number.

Instead of panicking, set errno to 0. POSIX requires strerror(3p) to have useful function when its argument is 0.

This is not C, and error is the internal value of an Err variant of a Result, which is an io::Error. Passing this function an io::Error that has no corresponding RawOsError is not the equivalent of passing it 0, it’s more like passing the function a null terminator.

To save work with inevitable localization stuff it may be wise to use strerror_l(3p) instead with the local locale - if we want to support locales.

From strerror(3p):

DESCRIPTION
       For strerror(): The functionality described on this reference page is
       aligned with the ISO C standard. Any conflict between the requirements
       described here and the ISO C standard is unintentional. This volume of
       POSIX.1‐2017 defers to the ISO C standard.

       The strerror() function shall map the error number in errnum to a
       locale-dependent error message string and shall return a pointer to it.
       Typically, the values for errnum come from errno, but strerror() shall
       map any value of type int to a message.

> For consistency with C conventions error should be errno. This is not C and `err` is not a number. > Instead of panicking, set errno to 0. POSIX requires strerror(3p) to have useful function when its argument is 0. This is not C, and error is the internal value of an `Err` variant of a `Result`, which is an `io::Error`. Passing this function an `io::Error` that has no corresponding `RawOsError` is not the equivalent of passing it `0`, it’s more like passing the function a null terminator. > To save work with inevitable localization stuff it may be wise to use strerror_l(3p) instead with the local locale - if we want to support locales. From strerror(3p): ``` DESCRIPTION For strerror(): The functionality described on this reference page is aligned with the ISO C standard. Any conflict between the requirements described here and the ISO C standard is unintentional. This volume of POSIX.1‐2017 defers to the ISO C standard. The strerror() function shall map the error number in errnum to a locale-dependent error message string and shall return a pointer to it. Typically, the values for errnum come from errno, but strerror() shall map any value of type int to a message. ```

emma commented

2024-03-02 14:24:18 -07:00

For consistency with C conventions error should be errno.

This is not C and err is not a number.

I misunderstood. I will change the error variable to errno if it’ll be more understandable to you.

> > For consistency with C conventions error should be errno. > > This is not C and err is not a number. I misunderstood. I will change the `error` variable to `errno` if it’ll be more understandable to you.

trinity commented

2024-03-02 14:52:26 -07:00

I'm glad you showed me the strerrror(3p) locale thing.

In person we discussed errno being fine and the panic! being technically unreachable - though I still believe 0 should be passed to strerror(3) instead of a panic.

I'm glad you showed me the strerrror(3p) locale thing. In person we discussed `errno` being fine and the `panic!` being technically unreachable - though I still believe 0 should be passed to strerror(3) instead of a panic.

emma commented

2024-03-18 21:33:26 -06:00

I have decided to make 0 work but as soon as we can put manual compiler errors in the library we will do that.

I have decided to make `0` work but as soon as we can put manual compiler errors in the library we will do that.

emma marked this conversation as resolved

trinity reviewed 2024-03-02 03:47:20 -07:00

src/strerror.rs Outdated

						
				@ -0,0 +20,4 @@

					/* Get a CStr from the error message so that it’s referenced and then

					 * convert it to an owned value. If the string is not valid UTF-8, return

					 * that error instead. */

					match unsafe { CStr::from_ptr(strerror(error)) }.to_str() {

trinity commented

2024-03-02 03:47:20 -07:00

You should note somewhere that this can return an empty string in case of an error.

emma commented

2024-03-02 09:53:42 -07:00

It cannot do this. It either returns a String derived from the RawOsError or, in the case that the locale-specified error string is not valid UTF-8, returns a String derived from a Utf8Error.

It cannot do this. It either returns a `String` derived from the `RawOsError` or, in the case that the locale-specified error string is not valid UTF-8, returns a `String` derived from a `Utf8Error`.

emma marked this conversation as resolved

emma added 1 commit 2024-03-02 10:17:26 -07:00

Makefile: better deps; fop(1), hru(1), strerror(3): changed strerror wrapper function name c392dbc680

emma added 1 commit 2024-03-03 17:35:49 -07:00

changes redundant .unwrap_or_else(panic!()) to .unwrap() 8193e471f0

emma added 1 commit 2024-03-09 12:42:00 -07:00

Makefile: DESTDIR 2ad7140e1e

emma added 1 commit 2024-03-09 22:03:16 -07:00

Makefile: even more changes b2d56bbc9a

emma added 1 commit 2024-03-18 20:47:22 -06:00

strerror(3): changed panic to 0 b356ac522f

emma requested review from trinity 2024-03-18 21:06:24 -06:00

emma added 2 commits 2024-03-18 21:32:08 -06:00

strerror(3): created strerror() method 1891c3e1aa

hru(1), fop(1): changed to reflect strerror changes d87b5d0958

trinity requested changes 2024-03-22 06:20:48 -06:00

trinity left a comment

I"ve suggested some changes but I really like the intent.

Makefile Outdated

						
				@ -11,3 +13,4 @@

				.PRAGMA: posix_202x # future POSIX standard support à la pdpmake(1)

				.PRAGMA: command_comment # breaks without this?

				DESTDIR=./dist

trinity commented

2024-03-10 09:17:18 -06:00

For readability, spaces between the assignment operators and their names and values would be nice. e.g. DESTDIR = ./dist (though I would forego the ./ as Makefiles are conventionally run relative to $PWD anyway).

For readability, spaces between the assignment operators and their names and values would be nice. e.g. `DESTDIR = ./dist` (though I would forego the `./` as Makefiles are conventionally run relative to `$PWD` anyway).

emma marked this conversation as resolved

Makefile Outdated

						
				@ -32,3 +42,1 @@

					mkdir -p dist/bin dist/share/man/man1

					cp build/bin/* dist/bin/

					cp docs/*.1 dist/share/man/man1/

					mkdir -p $(DESTDIR)$(PREFIX)/bin $(DESTDIR)$(PREFIX)/share/man/man1

trinity commented

2024-03-10 09:21:12 -06:00

Consecutive directory delimiters are ignored when evaluating UNIX paths; /// is an unnecessarily wordy equivalent to / when used as a pathname. $(DESTDIR)/$(PREFIX)/bin and so on is preferable, though, to ensure that hasty modifications won't write to a distbuildbin or something. Mistakes happen!

The names PREFIX and DESTDIR sound equivalent to me. On face value I would expect the prefix to come first.

Consecutive directory delimiters are ignored when evaluating UNIX paths; `///` is an unnecessarily wordy equivalent to `/` when used as a pathname. `$(DESTDIR)/$(PREFIX)/bin` and so on is preferable, though, to ensure that hasty modifications won't write to a `distbuildbin` or something. Mistakes happen! The names `PREFIX` and `DESTDIR` sound equivalent to me. On face value I would expect the prefix to come first.

emma commented

2024-03-24 13:21:54 -06:00

Historically, $DESTDIR is a directory inside the tree (in this case, dist/) to which programs are installed in a phony root filesystem. Then, when make install is called, the $PREFIX is already baked into $DESTDIR and you can copy it straight into the root filesystem.

[Historically](https://people.freebsd.org/~rafan/ph/porting-prefix.html), `$DESTDIR` is a directory inside the tree (in this case, `dist/`) to which programs are installed in a phony root filesystem. Then, when `make install` is called, the `$PREFIX` is already baked into `$DESTDIR` and you can copy it straight into the root filesystem.

emma marked this conversation as resolved

Makefile

						
				@ -37,3 +47,2 @@

				install: dist

					mkdir -p $(PREFIX)

					cp -r dist/* $(PREFIX)/

					cp -r $(DESTDIR)/* /

trinity commented

2024-03-10 09:22:10 -06:00

Installation to a given PREFIX should be possible.

Installation to a given `PREFIX` should be possible.

emma commented

2024-03-24 13:05:51 -06:00

That is what this does

emma commented

2024-03-29 16:31:43 -06:00

@trinity if you have no more questions, may you approve this for merging?

emma marked this conversation as resolved

src/test.rs

						
				@ -0,0 +1,10 @@

				extern crate strerror;

trinity commented

2024-03-22 06:20:00 -06:00

What's the purpose of this file?

emma commented

2024-03-24 13:05:42 -06:00

Mistake.

emma marked this conversation as resolved

trinity added 1 commit 2024-03-23 18:18:16 -06:00

Merge branch 'main' into strerror 7e45af1bca

emma added 1 commit 2024-03-24 13:17:15 -06:00

Revert "Merge branch 'main' into strerror (not PGP signed)" 31ae3d1997

This reverts commit 7e45af1bca, reversing
changes made to d87b5d0958.

emma commented

2024-03-24 13:18:30 -06:00

@trinity: in the future, do not make commits without PGP signing them, and do not merge main into branches that have pull requests open to main.

emma added 1 commit 2024-03-24 13:29:26 -06:00

Makefile: directory specification changes c75bb93001

trinity commented

2024-03-24 18:49:15 -06:00

@trinity: in the future, do not make commits without PGP signing them, and do not merge main into branches that have pull requests open to main.

I think I misclicked, I don't remember doing that and I wouldn't commit to a branch I didn't create. Sorry about that.

> @trinity: in the future, do not make commits without PGP signing them, and do not merge main into branches that have pull requests open to main. I think I misclicked, I don't remember doing that and I wouldn't commit to a branch I didn't create. Sorry about that.

trinity reviewed 2024-03-29 18:53:27 -06:00

docs/mm.1

						
				@ -1,76 +0,0 @@

				.\" Copyright (c) 2024 DTB <trinity@trinity.moe>

trinity commented

2024-03-29 18:53:27 -06:00

Would merging this branch nuke mm(1)?

emma commented

2024-03-29 19:11:37 -06:00

I think so, I can fix it though. Just approve it whenever you think it’s ready.

emma marked this conversation as resolved

emma commented

2024-03-29 19:27:09 -06:00

Merged.

emma closed this pull request

2024-03-29 19:27:09 -06:00

trinity commented

2024-03-30 12:59:07 -06:00

I gave the okay verbally as I was too tired to approve it digitally.

Pull request closed

This pull request cannot be reopened because the branch was deleted.

Sign in to join this conversation.

No reviewers

No Label

No Milestone

No project

No Assignees

2 Participants

Notifications

Due Date

The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: bonsai/harakit#76

				`@ -1,76 +0,0 @@`
				`.\" Copyright (c) 2024 DTB <trinity@trinity.moe>`

Added strerror(3) as a Rust library #76

Pull request closed

Added `strerror(3)` as a Rust library #76