solarish stat following-up, supports for readdir.

[devnexen]

No description provided.

[devnexen]

@rustbot ready

[RalfJung]

Please don't duplicate the entire assert. Instead, you can do something like

let expected_err = if cfg!(any(target_os = "solaris", target_os = "illumos")) { ErrorKind::AlreadyExists } else { ErrorKind::DirectoryNotEmpty };
assert_eq!(expected_err, remove_dir(&dir_path).unwrap_err().kind());

[RalfJung]

This assertion seems to fail on illumos now.

[devnexen]

Locally it passes tough, dunno if this is due to native vs non native run.

[RalfJung]

"Locally" meaning on an Illumos system? If you are passing through host errors, and host errors differ between OSes, then Miri will have to adjust the errors.

This likely also means that on your system, the tests will fail if you run them with --target x86_64-unknown-linux-gnu?

[devnexen]

"Locally" meaning on an Illumos system?

Yes.

If you are passing through host errors, and host errors differ between OSes, then Miri will have to adjust the errors.

This likely also means that on your system, the tests will fail if you run them with --target x86_64-unknown-linux-gnu?

And yes.

[RalfJung]

Right, so we have two options:

• we relax the tests, and accept that Miri behaves differently on different hosts since we just "pass through" errors
• we make some attempts at error normalization

I worry that the latter will be an endless can of worms, we'll never get it "right". So how bad would it be to do the former? Does anyone really rely on the specific errors being emitted?

@rust-lang/miri what do you think?

[saethlin]

Does anyone really rely on the specific errors being emitted?

The standard library does/has relied on specific error codes being returned by standard library functions, and has panicked or hit a debug assertion when an unexpected error code is produced.

I thought "pass through to the host" was something we explicitly avoided?

[RalfJung]

I thought "pass through to the host" was something we explicitly avoided?

For IO functions it is unclear what else we could practically do. We already pass through the actual file system of the host (i.e., this anyway requires disabling isolation), also passing through the errors codes doesn't make a big difference any more at that point.

The standard library does/has relied on specific error codes being returned by standard library functions, and has panicked or hit a debug assertion when an unexpected error code is produced.

Yes, but AFAIK this usually only checks that the error is some code allowed by POSIX -- whereas here we are apparently talking about POSIX allowing different codes, and different Unixes indeed using different codes.

[saethlin]

Oh dear, I see what's up here. Solarish is returning EEXIST for an attempt to remove a non-empty directory, because the POSIX description is "File Exists". But then std translates this error code to ErrorKind::AlreadyExists, adding the "already" which at least in this specific case, is definitely wrong. But when we display an io::Error, we punt to strerror which should only say "File exists" so we loop back to reasonable behavior.

So I'm okay with some non-portability here. Is not the first time the ErrorKind translation has been rough.

[RalfJung]

Okay, so let's keep passing through the host error, and let's make the test accept both AlreadyExists and NotEmpty.

[RalfJung]

@rustbot author

[devnexen]

@rustbot ready

[RalfJung]

Please don't duplicate this code. Instead, use this function to implement project_field_named and projectable_has_field.

[RalfJung]

Suggested change

	let fields = &dirent64_layout.fields;
	let fcount = (*fields).count().checked_sub(1).expect("invalid count");
	let d_name_offset = (*fields).offset(fcount).bytes();
	let last_field = dirent64_layout.fields.count().strict_sub(1);
	let d_name_offset = (*fields).offset(last_field).bytes();

[RalfJung]

@rustbot author

[devnexen]

I think I know why there is weird commit hours from me, if I let the VM/host hibernate for so many hours, the time from VM does not catch up once I wake it up.

[devnexen]

@rustbot ready

[devnexen]

@RalfJung before I squash do you like it better now ?

[RalfJung]

My proposal was to add a type_name: &str argument to the linux_solarish_readdir64 function.

[RalfJung]

oops seems like I never actually posted that proposal here. Sorry for that! I was sure I submitted this, but apparently I replaced it by the question about readdir64 on Solarish.

[devnexen]

oops seems like I never actually posted that proposal here. Sorry for that! I was sure I submitted this, but apparently I replaced it by the question about readdir64 on Solarish.

no worries, updating :)

[RalfJung]

Btw, please avoid rebasing over the latest master branch when there are no conflicts. Github is terrible at dealing with force pushes during review, so the more you can avoid force pushes outside of squashing, the better. :)

[RalfJung]

Suggested change

	if let Some(_f) = self.try_project_field_named(base, name).unwrap() {
	return true;
	}
	false
	self.try_project_field_named(base, name).unwrap().is_some()

[RalfJung]

Apart from one final nit, this looks great, thanks! Please squash the commits, then we can land this. Please use the --keep-base flag when squashing so that the force-push diff is easier to review. Then write @rustbot ready after you pushed the squashed PR.

@rustbot author

[devnexen]

Btw, please avoid rebasing over the latest master branch when there are no conflicts. Github is terrible at dealing with force pushes during review, so the more you can avoid force pushes outside of squashing, the better. :)

ok will try to remember that.

[devnexen]

@rustbot ready