On Mon, 2011-11-28 at 19:26 -0800, Nicholas A. Bellinger wrote:
> On Tue, 2011-11-29 at 03:44 +0100, Jörn Engel wrote:
> > On Mon, 28 November 2011 17:13:16 -0800, Nicholas A. Bellinger wrote:
> > > On Mon, 2011-11-28 at 01:05 -0800, Nicholas A. Bellinger wrote:
> > > > On Thu, 2011-11-24 at 02:05 +0100, Jörn Engel wrote:
> > > > > A reader should spend an extra moment whenever noticing a cast.
> > >
> > > > > @@ -821,6 +820,7 @@ int iscsi_target_setup_login_socket(
> > > > > /*
> > > > > * Set SO_REUSEADDR, and disable Nagel Algorithm with TCP_NODELAY.
> > > > > */
> > > > > + /* FIXME: Someone please explain why this is endian-safe */
> > > > > opt = 1;
> > > > > if (np->np_network_transport == ISCSI_TCP) {
> > > > > ret = kernel_setsockopt(sock, IPPROTO_TCP, TCP_NODELAY,
> > > > > @@ -832,6 +832,7 @@ int iscsi_target_setup_login_socket(
> > > > > }
> > > > > }
> > > > >
> > > > > + /* FIXME: Someone please explain why this is endian-safe */
> > > > > ret = kernel_setsockopt(sock, SOL_SOCKET, SO_REUSEADDR,
> > > > > (char *)&opt, sizeof(opt));
> > > > > if (ret < 0) {
> > > >
> > > > Another good catch. Changing opt to be a single char value with
> > > > kernel_setsockopt().
> > >
> > > Sorry, my last change here to use a single char actually actually broke
> > > iscsi-target in lio-core, so reverting this for the moment.
> > >
> > > All of the other net/ users of kernel_setsockopt() seem to be doing the
> > > same thing with int when enabling a '1' value opt. Is the caller really
> > > not endian safe here..? I don't recall having an issue with big-endian
> > > here in the recent past, but i'll look at this with pseries soon to
> > > double check since it's been mentioned.
> >
> > I've written a quick test:
> >
> > #include <stdio.h>
> > int main(void)
> > {
> > int i, *pi;
> > char *pc;
> >
> > for (i = 0; i < 2; i++) {
> > pi = &i;
> > pc = (char *)pi;
> > *pc;
> > printf("%d %d %d %p %p\n", i, *pi, *pc, pi, pc);
> > }
> > }
> >
> > Results on x86:
> > 0 0 0 0x7fffccb1b3ec 0x7fffccb1b3ec
> > 1 1 1 0x7fffccb1b3ec 0x7fffccb1b3ec
> >
> > Results on PowerPC:
> > 0 0 0 2ff22ba0 2ff22ba0
> > 1 1 0 2ff22ba0 2ff22ba0
> >
> > Unless having a different result depending on endianness is the
> > desired behaviour, I would call this a problem. ;)
> >
>
> If kernel_setsockopt() has a problem with accepting int to enable
> boolean *opt usage, then the other mainline usage needs to be fixed as
> well. (DaveM CC'ed)
>
> Anyways, will get this verified soon with iscsi-target on ppc64.
>
So after setting TCP_NODELAY with kernel_setsockopt() using the original
'int opt = 1' value, calling kernel_getsockopt() does return the same
value after enabling TCP_NODELAY on both architectures:
x86_64:
[67418.250760] kernel_getsockopt: TCP_NODELAY: 0x00000001
ppc64:
[ 2263.333262] kernel_getsockopt: TCP_NODELAY: 0x00000001
diff --git a/drivers/target/iscsi/iscsi_target_login.c b/drivers/target/iscsi/iscsi_target_login.c
index 373b0cc..b57395b 100644
--- a/drivers/target/iscsi/iscsi_target_login.c
+++ b/drivers/target/iscsi/iscsi_target_login.c
@@ -823,6 +823,8 @@ int iscsi_target_setup_login_socket(
/* FIXME: Someone please explain why this is endian-safe */
opt = 1;
if (np->np_network_transport == ISCSI_TCP) {
+ int foo, foo_len;
+
ret = kernel_setsockopt(sock, IPPROTO_TCP, TCP_NODELAY,
(char *)&opt, sizeof(opt));
if (ret < 0) {
@@ -830,6 +832,14 @@ int iscsi_target_setup_login_socket(
" failed: %d\n", ret);
goto fail;
}
+
+ foo_len = sizeof(foo);
+ ret = kernel_getsockopt(sock, IPPROTO_TCP, TCP_NODELAY,
+ (char *)&foo, &foo_len);
+ if (ret < 0) {
+ printk("kernel_getsockopt failed: %d\n", ret);
+ }
+ printk("kernel_getsockopt: TCP_NODELAY: 0x%08x\n", foo);
}
/* FIXME: Someone please explain why this is endian-safe */
So AFAICT kernel_[get,set]sockopt() is already handling the endianness
for us here. That said, I'll go ahead and remove the extra FIXMEs added
with this patch unless you can show there really is a problem that needs
addressing.
Thanks,
--nab
--
To unsubscribe from this list: send the line "unsubscribe target-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
