Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

syscall: ztypes_linux_s390x.go: struct members with just underscore as names #18628

Closed
sharkcz opened this issue Jan 12, 2017 · 11 comments
Closed

Comments

@sharkcz
Copy link

sharkcz commented Jan 12, 2017

Structure members in syscall/ztypes_linux_s390x.go that should have a underscore in their names contain only the underscore as the name. It results in error like

# github.com/vishvananda/netlink/nl
../../rpmbuild/BUILDROOT/golang-github-vishvananda-netlink-0-0.10.gite73bad4.fc26.s390x/usr/share/gocode/src/github.com/vishvananda/netlink/nl/nl_linux_test.go:34: msg.X__ifi_pad undefined (type *IfInfomsg has no field or method X__ifi_pad)
FAIL	github.com/vishvananda/netlink/nl [build failed]

For a downstream bug please see https://bugzilla.redhat.com/show_bug.cgi?id=1404679

https://github.com/golang/sys/blob/master/unix/ztypes_linux_s390x.go from the standalone sys package is wrong as well.

What version of Go are you using (go version)?

git

What operating system and processor architecture are you using (go env)?

Linux/s390x

What did you do?

compile eg. golang-github-vishvananda-netlink library

What did you expect to see?

type IfInfomsg struct {
		Family     uint8
		X__ifi_pad uint8
		Type       uint16
		Index      int32
		Flags      uint32
		Change     uint32
	}

for example on https://golang.org/src/syscall/ztypes_linux_ppc64.go

The corresponding kernel structure is defined in include/uapi/linux/rtnetlink.h as

struct ifinfomsg {
        unsigned char   ifi_family;
        unsigned char   __ifi_pad;
        unsigned short  ifi_type;               /* ARPHRD_* */
        int             ifi_index;              /* Link index   */
        unsigned        ifi_flags;              /* IFF_* flags  */
        unsigned        ifi_change;             /* IFF_* change mask */
};

What did you see instead?

from https://golang.org/src/syscall/ztypes_linux_s390x.go

type IfInfomsg struct {
		Family uint8
		_      uint8
		Type   uint16
		Index  int32
		Flags  uint32
		Change uint32
    }
@sharkcz
Copy link
Author

sharkcz commented Jan 12, 2017

The IfInfomsg structure is only a sample, there are more with this error.

@randall77
Copy link
Contributor

This looks intentional. Field names that start with X_ are replaced with _ by src/syscall/mkpost.go:42-43.

It is just a padding field - why do you need to refer to it?

@mundaym
Copy link
Member

mundaym commented Jan 12, 2017

The fields were omitted from the API exposed on s390x because they weren't considered to have any valid uses. If that assumption is wrong then I'm happy to expose them in the x/sys/unix package at least (the syscall package is supposed to be frozen and shouldn't be used).

The __ifi_pad is just padding. It isn't in the man page for rtnetlink. There the struct is described as:

struct ifinfomsg {
    unsigned char  ifi_family; /* AF_UNSPEC */
    unsigned short ifi_type;   /* Device type */
    int            ifi_index;  /* Interface index */
    unsigned int   ifi_flags;  /* Device flags  */
    unsigned int   ifi_change; /* change mask */
};

In my opinion the netlink library should therefore not rely on its presence. Unfortunately it is not just a one line change to do this, the tests will have to be rethought a little as the padding field appears not to be cleared, so the equality checks fail (here is the code: https://github.com/vishvananda/netlink/blob/90b9ee53585fd2ab8360c6bde4c9f19f23169ac8/nl/nl_linux_test.go#L34).

@sharkcz
Copy link
Author

sharkcz commented Jan 12, 2017

We found at least 2 projects that are touching the padding fields, one is the mentioned netlink, another appeared right today

# github.com/pkg/sftp
../../BUILDROOT/golang-github-pkg-sftp-0-0.1.git8197a2e.fc26.s390x/usr/share/gocode/src/github.com/pkg/sftp/server_statvfs_linux.go:19: stat.Fsid.X__val undefined (type syscall.Fsid has no field or method X__val)
FAIL	github.com/pkg/sftp [build failed]

So my preference would be to add proper names also to the padding fields, because people do weird things :-)

@sharkcz
Copy link
Author

sharkcz commented Jan 12, 2017

Also I'm not sure if only the paddings are named with underscore or some valid members are misnamed. But a consistent solution across all arches would be appreciated.

@bradfitz
Copy link
Contributor

@sharkcz, that project was already fixed on Sep 30th 2016 via pkg/sftp@4d0e916 for duplicate bug #17298

@sharkcz
Copy link
Author

sharkcz commented Jan 12, 2017

Ah, thanks for, I'll ask our maintainers to update the sftp package. Dunno why they used older snapshot, although the package was introduced into Fedora yesterday.

@bradfitz bradfitz added this to the Unplanned milestone Jan 13, 2017
@bradfitz bradfitz changed the title syscall/ztypes_linux_s390x.go: struct members with just underscore as names syscall: ztypes_linux_s390x.go: struct members with just underscore as names Jan 13, 2017
@bradfitz
Copy link
Contributor

The meta issue is #18632.

@gopherbot
Copy link
Contributor

CL https://golang.org/cl/35262 mentions this issue.

@tklauser
Copy link
Member

The originally reported issue seems to be have been fixed in vishvananda/netlink@a1d6c19 as well.

Can we close this issue then?

@ianlancetaylor
Copy link
Contributor

I think so. Closing.

@golang golang locked and limited conversation to collaborators Jan 24, 2019
jaxesn pushed a commit to danbudris/go that referenced this issue Sep 9, 2022
Exposing this field on s390x improves compatibility with the other
linux architectures, all of which already expose it.

Fixes golang#18628 and updates golang#18632.

Change-Id: I08e8e1eb705f898cd8822f8bee0d61ce11d514b5
rcrozean pushed a commit to rcrozean/go that referenced this issue Oct 5, 2022
Exposing this field on s390x improves compatibility with the other
linux architectures, all of which already expose it.

Fixes golang#18628 and updates golang#18632.

Change-Id: I08e8e1eb705f898cd8822f8bee0d61ce11d514b5
rcrozean pushed a commit to rcrozean/go that referenced this issue Oct 12, 2022
Exposing this field on s390x improves compatibility with the other
linux architectures, all of which already expose it.

Fixes golang#18628 and updates golang#18632.

Change-Id: I08e8e1eb705f898cd8822f8bee0d61ce11d514b5
rcrozean pushed a commit to rcrozean/go that referenced this issue Oct 12, 2022
Exposing this field on s390x improves compatibility with the other
linux architectures, all of which already expose it.

Fixes golang#18628 and updates golang#18632.

Change-Id: I08e8e1eb705f898cd8822f8bee0d61ce11d514b5
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

7 participants