ndbm.h: Unsign dsize by changing it to unsigned int
AbandonedPublic
Actions

Authored by pfg on Apr 30 2017, 5:23 AM.

Details

Reviewers

delphij
emaste
ngie

Group Reviewers

manpages

Summary

According to POSIX 1003.1-2008, dsize should be size_t. Unfortunately this would break the ABI. Since size_t is unsigned, we can at least make it a unsigned so the types are more compatible and we avoid some implicit sign/unsigned casts.

According to opengrok, all uses involve assignments to/from unsigned variables so there would be no negative side-effects to this change..

Test Plan

Going through a tinderbox build now.

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 8990
Build 9387: CI src build	Jenkins
Build 9386: arc lint + arc unit

Event Timeline

pfg created this revision.Apr 30 2017, 5:23 AM

Herald added a subscriber: imp. · View Herald TranscriptApr 30 2017, 5:23 AM

Harbormaster completed remote builds in B8985: Diff 27855.Apr 30 2017, 5:23 AM

Harbormaster completed remote builds in B8985: Diff 27855.Apr 30 2017, 6:06 AM

Manual pages should be updated too.

And I don't see any reason to replace int here with something else that is not POSIX compliant. Why not just replace it with size_t?

This revision now requires changes to proceed.Apr 30 2017, 6:09 AM

In D10544#218514, @delphij wrote:

Manual pages should be updated too.

Ah yes, dbm(3).

And I don't see any reason to replace int here with something else that is not POSIX compliant. Why not just replace it with size_t?

I was trying to avoid breaking the ABI. I did a second check through opengrok and it appears we can indeed change it.

Take 2: Comply with POSIX.

This can't be MFC'd.

Herald added a reviewer: manpages. · View Herald TranscriptApr 30 2017, 5:07 PM

Harbormaster completed remote builds in B8990: Diff 27864.Apr 30 2017, 5:43 PM

In D10544#218590, @pfg wrote:

Take 2: Comply with POSIX.

This can't be MFC'd.

No, libc ABI must NOT be broken regardless if it's MFC'ed, you should implement versioned compatibility shims.

In D10544#218735, @delphij wrote:

In D10544#218590, @pfg wrote:

Take 2: Comply with POSIX.

This can't be MFC'd.

No, libc ABI must NOT be broken regardless if it's MFC'ed, you should implement versioned compatibility shims.

That's just not feasible: the datum struct touches everything in dbm(3).

An unsigned int would not break the ABI as we wouldn't be changing the struct size, the other alternative is to leave it untouched and hope that no one uses it.

In D10544#218736, @pfg wrote:

In D10544#218735, @delphij wrote:

In D10544#218590, @pfg wrote:

Take 2: Comply with POSIX.

This can't be MFC'd.

No, libc ABI must NOT be broken regardless if it's MFC'ed, you should implement versioned compatibility shims.

That's just not feasible: the datum struct touches everything in dbm(3).

I might be missing something, but can't you just create wrappers around the new dbm(3) APIs to implement old binary interfaces, and __sym_compat these FBSD_1.0 symbols?

In D10544#218740, @delphij wrote:

In D10544#218736, @pfg wrote:

In D10544#218735, @delphij wrote:

In D10544#218590, @pfg wrote:

Take 2: Comply with POSIX.

This can't be MFC'd.

No, libc ABI must NOT be broken regardless if it's MFC'ed, you should implement versioned compatibility shims.

That's just not feasible: the datum struct touches everything in dbm(3).

I might be missing something, but can't you just create wrappers around the new dbm(3) APIs to implement old binary interfaces, and __sym_compat these FBSD_1.0 symbols?

Yes but maybe I am the one missing something: (re-phrased) I can't wrap a struct without wrapping all the functions that use it. I would have to copy the library with the old struct and provide both implementations.
The struct is only used on libc/db/hash/ndbm.c but it still seems too much bloat to copy the old version for such a "minor" change.

I'd really prefer to drop the change altogether.

In D10544#218741, @pfg wrote:

In D10544#218740, @delphij wrote:

In D10544#218736, @pfg wrote:

In D10544#218735, @delphij wrote:

In D10544#218590, @pfg wrote:

Take 2: Comply with POSIX.

This can't be MFC'd.

No, libc ABI must NOT be broken regardless if it's MFC'ed, you should implement versioned compatibility shims.

That's just not feasible: the datum struct touches everything in dbm(3).

I might be missing something, but can't you just create wrappers around the new dbm(3) APIs to implement old binary interfaces, and __sym_compat these FBSD_1.0 symbols?

Yes but maybe I am the one missing something: I can't wrap a structure. I would have to copy the library with the old struct and provide both implementations.
The struct is only used on libc/db/hash/ndbm.c but it still seems too much bloat to copy the old version for such a "minor" change.

No, you don't have to. Basically what you need is to have something like:

typedef struct {
  void *dptr;
  int dsize;
} datum_44bsd;

Then, have some translators that basically translates datum from and to datum_44bsd? (I don't have an example off-hand but you may want to take a look at how fts(3) was done).

OK, it may be worth breaking the ABI but, for now, given that we will
have to carry the old datum for compatibility anyways, how about being
one bit more efficient?

pfg retitled this revision from ndbm.h: Unsign dsize by declaring it uint32_t. to ndbm.h: Unsign dsize by changing it do unsigned int.May 1 2017, 3:31 PM

pfg edited the summary of this revision. (Show Details)

Harbormaster completed remote builds in B9002: Diff 27890.May 1 2017, 4:11 PM

In D10544#218799, @pfg wrote:

OK, it may be worth breaking the ABI but, for now, given that we will
have to carry the old datum for compatibility anyways, how about being
one bit more efficient?

IMHO it doesn't make sense to diverge from both historical BSD API and POSIX at the same time.

This revision now requires changes to proceed.May 1 2017, 4:38 PM

Not much support for this change.

For the record, I'm ok with the change if sufficient compatibility shims are built in, like delphij@ requested.

The affected calls in libc are:

dbm_delete
dbm_fetch
dbm_store
xdr_datum

There aren't any consumers in kernel space.

There are some consumers in contrib/, but the number is relatively small:

amd
gcc
sendmail

Writing compat libcalls for the above items doesn't seem that difficult to do, and I think that your proposed change is a net-win.

In D10544#219148, @ngie wrote:

For the record, I'm ok with the change if sufficient compatibility shims are built in, like delphij@ requested.

The affected calls in libc are:

dbm_delete

dbm_fetch

dbm_store

xdr_datum

There aren't any consumers in kernel space.

There are some consumers in contrib/, but the number is relatively small:

amd

gcc

sendmail

Writing compat libcalls for the above items doesn't seem that difficult to do, and I think that your proposed change is a net-win.

To be clear: I'm endorsing the "let's move from int to size_t" change.

In D10544#219149, @ngie wrote:

In D10544#219148, @ngie wrote:

For the record, I'm ok with the change if sufficient compatibility shims are built in, like delphij@ requested.

The affected calls in libc are:

dbm_delete

dbm_fetch

dbm_store

xdr_datum

There aren't any consumers in kernel space.

There are some consumers in contrib/, but the number is relatively small:

amd

gcc

sendmail

Writing compat libcalls for the above items doesn't seem that difficult to do, and I think that your proposed change is a net-win.

To be clear: I'm endorsing the "let's move from int to size_t" change.

I wanted to keep it really simple; only unsigning without breaking the ABI. I won't go further.

However, if someone *is* going to break the ABI, then it may be worth looking at what the heimdal guys did. I think NetBSD adopted only some partial changes from there.