Question regarding alignment patch

Contrast

[url]http://cvsweb.mindrot.org/index.cgi/openssh/monitor_fdpass.c?r1=1.23;r2=1.24[/url]
with
[url]http://www.openbsd.org/cgi-bin/cvsweb/src/usr.bin/ssh/monitor_fdpass.c.diff?r1=1.14&r2=1.15[/url]

The original replaces cmsgbuf.tmp with cmsgbuf.buf, while the -portable
version *adds* cmsgbuf.buf but retains cmsgbuf.tmp.

I assume this was an oversight, and cmsgbuf.tmp should be removed?

DES
--
Dag-Erling Smørgrav - [email]des@des.no[/email]
_______________________________________________
openssh-unix-dev mailing list
[email]openssh-unix-dev@mindrot.org[/email]
[url]https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev[/url]

On Tue, Jul 29, 2008 at 04:53:38PM +0200, Dag-Erling Smørgrav wrote:[color=blue]
> Contrast
>
> [url]http://cvsweb.mindrot.org/index.cgi/openssh/monitor_fdpass.c?r1=1.23;r2=1.24[/url]
> with
> [url]http://www.openbsd.org/cgi-bin/cvsweb/src/usr.bin/ssh/monitor_fdpass.c.diff?r1=1.14&r2=1.15[/url]
>
> The original replaces cmsgbuf.tmp with cmsgbuf.buf, while the -portable
> version *adds* cmsgbuf.buf but retains cmsgbuf.tmp.
>
> I assume this was an oversight, and cmsgbuf.tmp should be removed?[/color]

Yes it certainly looks like an oversight.

On a somewhat related note, it seems that some platforms (NetBSD 1.6
at least) fail the recvmsg with EAGAIN making multiplexing fail with:

mm_receive_fd: recvmsg: Resource temporarily unavailable

Retrying on EAGAIN makes this work (well, pass the regress tests
anyway).

Index: monitor_fdpass.c
===================================================================
RCS file: /usr/local/src/security/openssh/cvs/openssh/monitor_fdpass.c,v
retrieving revision 1.26
diff -u -p -r1.26 monitor_fdpass.c
--- monitor_fdpass.c 27 Mar 2008 00:01:15 -0000 1.26
+++ monitor_fdpass.c 30 Jul 2008 00:31:16 -0000
@@ -51,7 +51,6 @@ mm_send_fd(int sock, int fd)
#ifndef HAVE_ACCRIGHTS_IN_MSGHDR
union {
struct cmsghdr hdr;
- char tmp[CMSG_SPACE(sizeof(int))];
char buf[CMSG_SPACE(sizeof(int))];
} cmsgbuf;
struct cmsghdr *cmsg;
@@ -76,7 +75,9 @@ mm_send_fd(int sock, int fd)
msg.msg_iov = &vec;
msg.msg_iovlen = 1;

- if ((n = sendmsg(sock, &msg, 0)) == -1) {
+ while ((n = sendmsg(sock, &msg, 0)) == -1 && errno == EAGAIN)
+ ;
+ if (n == -1) {
error("%s: sendmsg(%d): %s", __func__, fd,
strerror(errno));
return -1;
@@ -124,7 +125,9 @@ mm_receive_fd(int sock)
msg.msg_controllen = sizeof(cmsgbuf.buf);
#endif

- if ((n = recvmsg(sock, &msg, 0)) == -1) {
+ while ((n = recvmsg(sock, &msg, 0)) == -1 && errno == EAGAIN)
+ ;
+ if (n == -1) {
error("%s: recvmsg: %s", __func__, strerror(errno));
return -1;
}

--
Darren Tucker (dtucker at zip.com.au)
GPG key 8FF4FA69 / D9A3 86E9 7EEE AF4B B2D4 37C9 C982 80C7 8FF4 FA69
Good judgement comes with experience. Unfortunately, the experience
usually comes from bad judgement.
_______________________________________________
openssh-unix-dev mailing list
[email]openssh-unix-dev@mindrot.org[/email]
[url]https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev[/url]

On Wed, Jul 30, 2008 at 10:34:06AM +1000, Darren Tucker wrote:[color=blue]
> On Tue, Jul 29, 2008 at 04:53:38PM +0200, Dag-Erling Smørgrav wrote:[color=green]
> > Contrast
> >
> > [url]http://cvsweb.mindrot.org/index.cgi/openssh/monitor_fdpass.c?r1=1.23;r2=1.24[/url]
> > with
> > [url]http://www.openbsd.org/cgi-bin/cvsweb/src/usr.bin/ssh/monitor_fdpass.c.diff?r1=1.14&r2=1.15[/url]
> >
> > The original replaces cmsgbuf.tmp with cmsgbuf.buf, while the -portable
> > version *adds* cmsgbuf.buf but retains cmsgbuf.tmp.
> >
> > I assume this was an oversight, and cmsgbuf.tmp should be removed?[/color]
>
> Yes it certainly looks like an oversight.
>
> On a somewhat related note, it seems that some platforms (NetBSD 1.6
> at least) fail the recvmsg with EAGAIN making multiplexing fail with:[/color]

On another related note, if we reshuffle some variable declarations and
fiddle spacing we can cut the diff against OpenBSD by quite a bit (55
lines to 38 lines) and make the remainder more readable (this conficts
with my previous patch).

I'm not sure if there's a reason for the existing order (can't see one
from the CVS history). Damien?

Index: monitor_fdpass.c
===================================================================
RCS file: /usr/local/src/security/openssh/cvs/openssh/monitor_fdpass.c,v
retrieving revision 1.26
diff -u -p -r1.26 monitor_fdpass.c
--- monitor_fdpass.c 27 Mar 2008 00:01:15 -0000 1.26
+++ monitor_fdpass.c 30 Jul 2008 00:37:37 -0000
@@ -45,17 +45,16 @@ mm_send_fd(int sock, int fd)
{
#if defined(HAVE_SENDMSG) && (defined(HAVE_ACCRIGHTS_IN_MSGHDR) || defined(HAVE_CONTROL_IN_MSGHDR))
struct msghdr msg;
- struct iovec vec;
- char ch = '\0';
- ssize_t n;
#ifndef HAVE_ACCRIGHTS_IN_MSGHDR
union {
struct cmsghdr hdr;
- char tmp[CMSG_SPACE(sizeof(int))];
char buf[CMSG_SPACE(sizeof(int))];
} cmsgbuf;
struct cmsghdr *cmsg;
#endif
+ struct iovec vec;
+ char ch = '\0';
+ ssize_t n;

memset(&msg, 0, sizeof(msg));
#ifdef HAVE_ACCRIGHTS_IN_MSGHDR
@@ -99,10 +98,6 @@ mm_receive_fd(int sock)
{
#if defined(HAVE_RECVMSG) && (defined(HAVE_ACCRIGHTS_IN_MSGHDR) || defined(HAVE_CONTROL_IN_MSGHDR))
struct msghdr msg;
- struct iovec vec;
- ssize_t n;
- char ch;
- int fd;
#ifndef HAVE_ACCRIGHTS_IN_MSGHDR
union {
struct cmsghdr hdr;
@@ -110,6 +105,10 @@ mm_receive_fd(int sock)
} cmsgbuf;
struct cmsghdr *cmsg;
#endif
+ struct iovec vec;
+ ssize_t n;
+ char ch;
+ int fd;

memset(&msg, 0, sizeof(msg));
vec.iov_base = &ch;
@@ -128,6 +127,7 @@ mm_receive_fd(int sock)
error("%s: recvmsg: %s", __func__, strerror(errno));
return -1;
}
+
if (n != 1) {
error("%s: recvmsg: expected received 1 got %ld",
__func__, (long)n);
@@ -145,6 +145,7 @@ mm_receive_fd(int sock)
error("%s: no message header", __func__);
return -1;
}
+
#ifndef BROKEN_CMSG_TYPE
if (cmsg->cmsg_type != SCM_RIGHTS) {
error("%s: expected type %d got %d", __func__,

--
Darren Tucker (dtucker at zip.com.au)
GPG key 8FF4FA69 / D9A3 86E9 7EEE AF4B B2D4 37C9 C982 80C7 8FF4 FA69
Good judgement comes with experience. Unfortunately, the experience
usually comes from bad judgement.
_______________________________________________
openssh-unix-dev mailing list
[email]openssh-unix-dev@mindrot.org[/email]
[url]https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev[/url]

On Wed, 30 Jul 2008, Darren Tucker wrote:
[color=blue]
> On another related note, if we reshuffle some variable declarations and
> fiddle spacing we can cut the diff against OpenBSD by quite a bit (55
> lines to 38 lines) and make the remainder more readable (this conficts
> with my previous patch).
>
> I'm not sure if there's a reason for the existing order (can't see one
> from the CVS history). Damien?[/color]

No reason, feel free to commit your fixes.

-d
_______________________________________________
openssh-unix-dev mailing list
[email]openssh-unix-dev@mindrot.org[/email]
[url]https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev[/url]