Skip to content

use ssize_t in substdio - #84

Merged
DerDakon merged 8 commits into
masterfrom
substdio-ssize_t
Dec 4, 2020
Merged

DerDakon merged 8 commits into
masterfrom
substdio-ssize_t

Conversation

@DerDakon

Copy link
Copy Markdown
Member

No description provided.

@DerDakon

Copy link
Copy Markdown
Member Author

I wonder if we should go for C89 function prototypes here if we touch these lines anyway. A collision is a collision, it's not harder to solve with a C89 function prototype than with a K&R one.

@alanpost alanpost added this to the 1.08 milestone Aug 25, 2019
@alanpost

alanpost commented Aug 25, 2019 •

Copy link
Copy Markdown
Contributor

I wonder if we should go for C89 function prototypes here if we touch these lines anyway. A collision is a collision, it's not harder to solve with a C89 function prototype than with a K&R one.

I am broadly wondering the same. I was going to ask it as "What led you to decide not to also change the type of the various len function parameters to size_t?" as I find leaving them and fixing them both persuasive--in some sense it's a question of when.

If you wanted to stage a series of commits showing each step, we can squash them together after review and put them in 1.08, optionally leaving work (not squashing all commits) to 1.9. I definitely think we're doing ourselves a portability favor by cleaning up this subsystem, now that we're using system-provided function declarations for read and write.

@DerDakon
DerDakon force-pushed the substdio-ssize_t branch 3 times, most recently from 46f71eb to c5056eb Compare September 6, 2019 21:42
@DerDakon
DerDakon force-pushed the substdio-ssize_t branch 3 times, most recently from 0c80a67 to d5870d7 Compare May 14, 2020 16:00
@DerDakon
DerDakon force-pushed the substdio-ssize_t branch 2 times, most recently from 21a168f to ccab760 Compare May 22, 2020 22:08
@DerDakon
DerDakon force-pushed the substdio-ssize_t branch from ccab760 to 91b241e Compare June 3, 2020 18:14
@DerDakon
DerDakon force-pushed the substdio-ssize_t branch 2 times, most recently from 3b4afdf to 814ceb3 Compare June 15, 2020 18:28
@DerDakon
DerDakon requested a review from mbhangui June 15, 2020 18:29
@DerDakon
DerDakon force-pushed the substdio-ssize_t branch 2 times, most recently from daa71e5 to 7472c9a Compare November 9, 2020 16:49
@schmonz schmonz linked an issue Nov 20, 2020 that may be closed by this pull request

@josuah josuah left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like we have two great things here:

factorizing ad-hoc read/write wrappers, and conversion of register native type to ssize_t.

If we are ok with defines, then we reached destination.

If we wat to get rid of the defines later on, maybe using a global variable will do (there is a global variable qqt/qq already for instance), andthere is always one struct qmail per program, so this :

#define GEN_QMAILPUT_WRITE(qq) \
ssize_t qmail_put_write(int fd, const void *buf, size_t len) \
{ \
  qmail_put(qq,buf,len); \
  return len; \
}

Could become this:

ssize_t qmail_put_write(int fd, const void *buf, size_t len)
{
  qmail_put(qqt,buf,len);
  return len;
}

I'm doing a patch and testing it after this is merged.

Comment thread qmail-smtpd.c Outdated
Comment thread substdio.h Outdated
Comment thread qmail.c
Comment thread getln2.c

@schmonz schmonz left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When we merge this PR, I'll be seeing 0 compile warnings on macOS Big Sur. 😃

The abstraction used to be invoked equally for reads and writes, and breaking that symmetry by introducing a write-specific path turns this into rather a large change. I'd like help understanding the tradeoff here.

Prototypes for read() and write() are indeed distinct, and the abstraction has until now chosen to blur that distinction. It seems the write-specific macro and function become necessary when we decide to make the distinction explicit. What benefits do we expect to gain by doing this? (Inversely, as I noted on typedef struct substdio, what would we lose if we chose to continue not specifying our I/O operation's prototype?)

The generator macros for timeout{read,write}() need fixing. It's good they don't need an fd passed in, because some of the callers were even out of order ;-)

Comment thread substdi.c Outdated
Comment thread substdio.c Outdated
Comment thread substdio.h Outdated
Comment thread substdo.c

@schmonz schmonz left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Outstanding.

Adapt the prototypes of all op functions used in substdio to match the interface
of read() and write(). In summary this is:

- the return type is ssize_t
- the buffer is void* for read and const void* for write functions
- the length parameter is size_t
This is what would happen internally anyway, but this way many of the calls can
be optimized away as the string is often constant. It also avoids the code
duplication to implement the function itself.
From the OpenBSD man page of read()

  ...some platforms allow for nbytes to range between SSIZE_MAX and
  SIZE_MAX - 2, in which case the return value of an error-free read() may
  appear as a negative number distinct from -1.
If substdio_get() return -1 this would decrease the length setting in the passed
substdio.
Both styles were used in the codebase, stick to one that seems less fragile.
@DerDakon
DerDakon requested a review from schmonz December 4, 2020 16:27

@schmonz schmonz left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great. Let’s get this merged!

@DerDakon
DerDakon merged commit 80e4f07 into master Dec 4, 2020
@DerDakon
DerDakon deleted the substdio-ssize_t branch December 4, 2020 20:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Default to -Wall (but not -Werror)

5 participants