Ceph » CephFS

Anoop C S wrote:

ceph_fchown(), ceph_chown() and ceph_lchown() APIs recently changed (#71015) its data types for uid and gid arguments from int to uid_t and gid_t respectively causing problems with go-ceph builds against the main branch. See https://github.com/ceph/go-ceph/issues/1119 for more details on the failures.

Go (with cgo) does not support implicit type casting. All type conversions must be explicit. This means you need to specify the target type when converting values between different data types. Similarly in this case we always had an explicit type cast as below:

Ouch! That's bad. I see how this change caused inconvenience :/

ret := C.ceph_fchown(f.mount.mount, f.fd, C.int(user), C.int(group)) https://github.com/ceph/go-ceph/blob/master/cephfs/file.go#L287
ret := C.ceph_chown(mount.mount, cPath, C.int(user), C.int(group)) https://github.com/ceph/go-ceph/blob/master/cephfs/permissions.go#L29
ret := C.ceph_lchown(mount.mount, cPath, C.int(user), C.int(group)) https://github.com/ceph/go-ceph/blob/master/cephfs/permissions.go#L38

It is not an option to blindly change the above explicit type cast to C.uid_t and C.gid_t without sacrificing backward compatibility. Ideally libcephfs could provide something (like a new #define or so) to lookout for such otherwise difficult to detect changes so that language bindings like Go can cope with it in a cleaner fashion.

Would library version checks suffice? We are guilt of not doing that for quite some time.

Venky Shankar wrote in #note-2:

Anoop C S wrote:

It is not an option to blindly change the above explicit type cast to C.uid_t and C.gid_t without sacrificing backward compatibility. Ideally libcephfs could provide something (like a new #define or so) to lookout for such otherwise difficult to detect changes so that language bindings like Go can cope with it in a cleaner fashion.

Would library version checks suffice? We are guilt of not doing that for quite some time.

I think that would suffice but I suppose such a change comes under ABI and demands a major version bump.

Anoop C S wrote in #note-3:

Venky Shankar wrote in #note-2:

Anoop C S wrote:

It is not an option to blindly change the above explicit type cast to C.uid_t and C.gid_t without sacrificing backward compatibility. Ideally libcephfs could provide something (like a new #define or so) to lookout for such otherwise difficult to detect changes so that language bindings like Go can cope with it in a cleaner fashion.

Would library version checks suffice? We are guilt of not doing that for quite some time.

I think that would suffice but I suppose such a change comes under ABI and demands a major version bump.

There is a major, minor and "extra" component to the version. IMO, this qualifies for minor. Major would be like a change that totally breaks API/ABI.

Venky Shankar wrote in #note-4:

Anoop C S wrote in #note-3:

Venky Shankar wrote in #note-2:

Anoop C S wrote:

It is not an option to blindly change the above explicit type cast to C.uid_t and C.gid_t without sacrificing backward compatibility. Ideally libcephfs could provide something (like a new #define or so) to lookout for such otherwise difficult to detect changes so that language bindings like Go can cope with it in a cleaner fashion.

Would library version checks suffice? We are guilt of not doing that for quite some time.

I think that would suffice but I suppose such a change comes under ABI and demands a major version bump.

There is a major, minor and "extra" component to the version. IMO, this qualifies for minor. Major would be like a change that totally breaks API/ABI.

FWIW, I'm going to add some details I found around library versioning for the record.

Application Programming Interface https://autotools.info/glossary.html#glossary.api

The generic term API refers to all the interfaces (or points of contact) between one application and another.
When referring to libraries, the term takes the specific meaning of the set of functions, constants and variables
defined in the library's public headers, to be further transformed into an ABI during the build step. 

Application Binary Interface https://autotools.info/glossary.html#glossary.abi

With ABI we refer to the interface of a library, as exposed to its consumers (other libraries or
executables alike), after the build step. In case of software written in languages similar to C,
the ABI is comprised of at least the name of the functions, the type and order of their parameters,
the type and size of the data it exports, and the content, order and size of its structures. 

So the change from int to uid_t / gid_t definitely falls under the category of ABI changes.

The version [ Current(C).Revision(R).Age(A) ] attached to the library refers to its ABI but libcephfs has been versioned at 2.0.0 for a long time. Whenever the ABI changes we are bound to change the equivalent version (at least by incrementing Age even for compatible changes). This has not been the case for libcephfs because (I think) we tend to rely on just API versions defined via LIBCEPHFS_VER_MAJOR, LIBCEPHFS_VER_MINOR and LIBCEPHFS_VER_EXTRA.

It is easy to change the ABI without changing the API itself - like changing the type of parameter as we recently did with chown() APIs. Native users of the library may not have to change their code but expected sizes differ in these two versions. Nevertheless, here go-ceph (Go bindings for Ceph) failed to even build which IMO calls for a major version bump with minor and patch (extra) versions reset to 0.

ref:
https://autotools.info/libtool/version.html
https://www.gnu.org/software/libtool/manual/html_node/Updating-version-info.html#Updating-version-info
https://semver.org/#semantic-versioning-specification-semver

Anoop C S wrote in #note-5:

Venky Shankar wrote in #note-4:

Anoop C S wrote in #note-3:

Venky Shankar wrote in #note-2:

Anoop C S wrote:

It is not an option to blindly change the above explicit type cast to C.uid_t and C.gid_t without sacrificing backward compatibility. Ideally libcephfs could provide something (like a new #define or so) to lookout for such otherwise difficult to detect changes so that language bindings like Go can cope with it in a cleaner fashion.

Would library version checks suffice? We are guilt of not doing that for quite some time.

I think that would suffice but I suppose such a change comes under ABI and demands a major version bump.

There is a major, minor and "extra" component to the version. IMO, this qualifies for minor. Major would be like a change that totally breaks API/ABI.

FWIW, I'm going to add some details I found around library versioning for the record.

Application Programming Interface https://autotools.info/glossary.html#glossary.api
[...]

Application Binary Interface https://autotools.info/glossary.html#glossary.abi
[...]

So the change from int to uid_t / gid_t definitely falls under the category of ABI changes.

The version [ Current(C).Revision(R).Age(A) ] attached to the library refers to its ABI but libcephfs has been versioned at 2.0.0 for a long time. Whenever the ABI changes we are bound to change the equivalent version (at least by incrementing Age even for compatible changes). This has not been the case for libcephfs because (I think) we tend to rely on just API versions defined via LIBCEPHFS_VER_MAJOR, LIBCEPHFS_VER_MINOR and LIBCEPHFS_VER_EXTRA.

It is easy to change the ABI without changing the API itself - like changing the type of parameter as we recently did with chown() APIs. Native users of the library may not have to change their code but expected sizes differ in these two versions. Nevertheless, here go-ceph (Go bindings for Ceph) failed to even build which IMO calls for a major version bump with minor and patch (extra) versions reset to 0.

OK, so the reason I mentioned minor version bump is that for Go, the type conversion wasn't implicit and I presumed a warning was thrown during compile. However, you mentioned that the Go bindings even failed to build in this case. In that respect, I take my word back and bumping the major version is the way forward for this.

For the record the failure is captured in the go-ceph issue Anoop C S linked, but just so you don't have to go looking

*** running: go vet -tags=main,ceph_main,ceph_preview ./cephfs
# github.com/ceph/go-ceph/cephfs
# [github.com/ceph/go-ceph/cephfs]
vet: cephfs/file.go:287:69: cannot use _Ctype_int(user) (value of int32 type _Ctype_int) as _Ctype_uid_t value in variable declaration
*** ERROR: returned 1
*** running: go test -timeout 15m -v -count=1 -tags=main,ceph_main,ceph_preview -covermode=count -coverprofile=cephfs.cover.out -coverpkg=github.com/ceph/go-ceph/cephfs -o ./cephfs/cephfs.test ./cephfs
# github.com/ceph/go-ceph/cephfs
cephfs/file.go:287:99: cannot use _Ctype_int(user) (value of int32 type _Ctype_int) as _Ctype_uid_t value in variable declaration
cephfs/file.go:287:112: cannot use _Ctype_int(group) (value of int32 type _Ctype_int) as _Ctype_gid_t value in variable declaration
cephfs/permissions.go:29:67: cannot use _Ctype_int(user) (value of int32 type _Ctype_int) as _Ctype_uid_t value in variable declaration
cephfs/permissions.go:29:80: cannot use _Ctype_int(group) (value of int32 type _Ctype_int) as _Ctype_gid_t value in variable declaration
cephfs/permissions.go:38:68: cannot use _Ctype_int(user) (value of int32 type _Ctype_int) as _Ctype_uid_t value in variable declaration
cephfs/permissions.go:38:81: cannot use _Ctype_int(group) (value of int32 type _Ctype_int) as _Ctype_gid_t value in variable declaration
# github.com/ceph/go-ceph/cephfs [github.com/ceph/go-ceph/cephfs.test]
FAIL    github.com/ceph/go-ceph/cephfs [build failed]
cephfs/file.go:287:99: cannot use _Ctype_int(user) (value of int32 type _Ctype_int) as _Ctype_uid_t value in variable declaration
cephfs/file.go:287:112: cannot use _Ctype_int(group) (value of int32 type _Ctype_int) as _Ctype_gid_t value in variable declaration
cephfs/permissions.go:29:67: cannot use _Ctype_int(user) (value of int32 type _Ctype_int) as _Ctype_uid_t value in variable declaration
cephfs/permissions.go:29:80: cannot use _Ctype_int(group) (value of int32 type _Ctype_int) as _Ctype_gid_t value in variable declaration
cephfs/permissions.go:38:68: cannot use _Ctype_int(user) (value of int32 type _Ctype_int) as _Ctype_uid_t value in variable declaration
cephfs/permissions.go:38:81: cannot use _Ctype_int(group) (value of int32 type _Ctype_int) as _Ctype_gid_t value in variable declaration
*** ERROR: returned 1
*** ERROR: cephfs tests failed
make: *** [Makefile:215: test-containers-test] Error 1

These are fatal errors produced by the Go compile.

@Venky,
Given the nature of the change for #71015, I think we shouldn't go for backports.