New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

core: introduce (and fix) code to pass errno to other OSes #15495

Merged
merged 3 commits into from Jun 16, 2017

Conversation

Projects
None yet
3 participants
@wjwithagen
Contributor

wjwithagen commented Jun 5, 2017

  • Some of the errno conversions were lost in the conversion
    to Cmake config
  • rename ceph_to_host_errno to _ceph_to_hostos_errno
    to indicate that the errnos are define per OS.

This is the second part, creating the basics.
Next it needs to be glued into communication with the peers.

Signed-off-by: Willem Jan Withagen wjw@digiware.nl

@@ -493,11 +495,12 @@ struct errorcode32_t {
}
void encode(bufferlist &bl) const {
::encode(code, bl);
__s32 newcode = hostos_to_ceph_errno(code);
::encode(newcode, bl);

This comment has been minimized.

@liewegas

liewegas Jun 5, 2017

Member

This was teh weird missing piece in the earlier version. Do you mind separating it into a separate patch? Eveything else is fixing the build and cmake-related regresssions. This is a change in behavior so that the the server-side can also use non-Linux errno values.

This comment has been minimized.

@wjwithagen

wjwithagen Jun 5, 2017

Contributor

@liewegas
Sure, no problem.

@@ -538,6 +538,24 @@ set(libcommon_files
${auth_files}
${mds_files})
if(FREEBSD)
list(APPEND libcommon_files common/freebsd_errno.cc)
endif()

This comment has been minimized.

@tchaikov

tchaikov Jun 6, 2017

Contributor

nit, could use elseif as they are mutual exclusive.

This comment has been minimized.

@wjwithagen

wjwithagen Jun 6, 2017

Contributor

@tchaikov
Good suggestion, will take it along.

@wjwithagen wjwithagen requested review from tchaikov and liewegas Jun 6, 2017

// and the Linux error as value
// Use the fact that the arry is initialised per default on all 0's
// And we do not translate for 0's, but return the original value.
static __s32 hostos_to_ceph_conv[256] = {

This comment has been minimized.

@tchaikov

tchaikov Jun 6, 2017

Contributor

please make this a const also.

This comment has been minimized.

@wjwithagen

wjwithagen Jun 6, 2017

Contributor

@tchaikov
Will do

// And we do not translate for 0's, but return the original value.
static __s32 hostos_to_ceph_conv[256] = {
// FreeBSD errno Linux errno
H2C_ERRNO(EDEADLK, 35), /* Resource deadlock avoided */

This comment has been minimized.

@tchaikov

tchaikov Jun 6, 2017

Contributor

i think

[EDEADL] = 35,

is more straightforward.

This comment has been minimized.

@wjwithagen

wjwithagen Jun 6, 2017

Contributor

@tchaikov
I like the macro more, since modifying the operational code is than just replacing the macro.
BTW We need to use the FreeBSD errno defines, and that does not have EDEADL

if (err <256 && hostos_to_ceph_conv[err] !=0 ) {
err = hostos_to_ceph_conv[err];
}
return err * sign;

This comment has been minimized.

@tchaikov

tchaikov Jun 6, 2017

Contributor

please note that if errno <= 34, we just use its original value.

__s32 hostos_to_ceph_errno(__s32 r)
{
  int err = std::abs(r);
  if (err > 34 && err < 256) {
    err = hostos_to_ceph_conv[err];
  }
  return std::copysign(err, r);
}

This comment has been minimized.

@wjwithagen

wjwithagen Jun 6, 2017

Contributor

@tchaikov
Not quite....

#define EDEADLK         11              /* Resource deadlock avoided */
                                        /* 11 was EAGAIN */

So those got swapped in FreeBSD.
And all values that are not modified are 0, and are thus passed without modification.
So no special need to check for < 34
But we do need to check if the replacement is != 0 before replacing.

Now if the array conversion is to your liking, I would also change the original function: ceph_to_hostos_errno() to work in the same way.

@wjwithagen wjwithagen changed the title from core: [DNM] introduce (and fix) code to pass errno to other OSes to core: introduce (and fix) code to pass errno to other OSes Jun 8, 2017

@wjwithagen

This comment has been minimized.

Contributor

wjwithagen commented Jun 8, 2017

@liewegas @tchaikov
This should be more or less the PR for this, plus/minus some rebasing and or change requests.

{
int sign = (r<0?-1:1);
int err = std::abs(r);
if (err <256 && hostos_to_ceph_conv[err] !=0 ) {

This comment has been minimized.

@tchaikov

tchaikov Jun 9, 2017

Contributor

add space around operator.

// converts from linux errno values to host values
__s32 ceph_to_hostos_errno(__s32 r)
{
int sign = (r<0?-1:1);

This comment has been minimized.

@tchaikov

tchaikov Jun 9, 2017

Contributor

no need to calc the sign. std::copysign(err, r); would suffice

This comment has been minimized.

@wjwithagen

wjwithagen Jun 9, 2017

Contributor

@tchaikov
I looked at that function, but it seemed to me that it was floating point only
And mostly delt with things as NAN.
OTOH, the implementation does exactly what I do.

// converts Host OS errno values to linux/Ceph values
__s32 hostos_to_ceph_errno(__s32 r)
{
int sign = (r<0?-1:1);

This comment has been minimized.

@tchaikov

tchaikov Jun 9, 2017

Contributor

ditto.

wjwithagen added some commits Jun 5, 2017

core: introduce (and fix) code to pass errno to other OSes
 - Some of the errno conversions were lost in the conversion
   to Cmake config
 - rename ceph_to_host_errno to _ceph_to_hostos_errno
   to indicate that the errnos are define per OS.

This is the second part, creating the basics.
Next it needs to be glued into communication with the peers.

Signed-off-by: Willem Jan Withagen <wjw@digiware.nl>
common/freebsd_errno.cc: ceph_to_host and host_toc_ceph errno transla…
…tion

Signed-off-by: Willem Jan Withagen <wjw@digiware.nl>
common: add empty hostos_to_ceph_errno(), to allow linking to work
Signed-off-by: Willem Jan Withagen <wjw@digiware.nl>
@liewegas

This comment has been minimized.

Member

liewegas commented Jun 16, 2017

retest this please

@liewegas liewegas merged commit bcc4f9b into ceph:master Jun 16, 2017

5 of 6 checks passed

default Build finished.
Details
Signed-off-by all commits in this PR are signed
Details
Unmodifed Submodules submodules for project are unmodified
Details
Unmodified Submodules submodules for project are unmodified
Details
arm64 make check arm64 make check succeeded
Details
make check make check succeeded
Details
@wjwithagen

This comment has been minimized.

Contributor

wjwithagen commented Jun 16, 2017

@liewegas
Arrgh, still a syntax error sneaked in... :(
Will submit fix

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment