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: error in definition of constant RTNLGRP_IPV4_IFADDR #15080

Closed
inoyatov opened this issue Apr 3, 2016 · 7 comments
Closed

syscall: error in definition of constant RTNLGRP_IPV4_IFADDR #15080

inoyatov opened this issue Apr 3, 2016 · 7 comments

Comments

@inoyatov
Copy link

inoyatov commented Apr 3, 2016

Please answer these questions before submitting your issue. Thanks!

  1. What version of Go are you using (go version)?
    1.6
  2. What operating system and processor architecture are you using (go env)?
    Ubuntu 14.04 64bit
  3. What did you do?
    Definition of RTNLGRP_IPV4_IFADDR = 0x5 is incorrect from syscall.go. Because C language definition of this variable is equal to #define RTMGRP_IPV4_IFADDR 0x10 according here rtnetlink.h

I cannot get syscall.RTM_NEWADD message due to this error. I did not create runnable code since to simulate you need IP address change. However this is my code which can be executed.

package main

import (
    "fmt"
    "syscall"
)

func main() {
    l, _ := ListenNetlink()

    for {
        msgs, err := l.ReadMsgs()
        if err != nil {
            fmt.Println("Could not read netlink: %s", err)
        }

        for _, m := range msgs {
            if IsNewAddr(&m) {
                fmt.Println("New Addr")
            }

            if IsDelAddr(&m) {
                fmt.Println("Del Addr")
            }
        }
    }
}

type NetlinkListener struct {
    fd int
    sa *syscall.SockaddrNetlink
}

func ListenNetlink() (*NetlinkListener, error) {
    groups := syscall.RTNLGRP_IPV4_IFADDR

    s, err := syscall.Socket(syscall.AF_NETLINK, syscall.SOCK_DGRAM,
        syscall.NETLINK_ROUTE)
    if err != nil {
        return nil, fmt.Errorf("socket: %s", err)
    }

    saddr := &syscall.SockaddrNetlink{
        Family: syscall.AF_NETLINK,
        Pid:    uint32(0),
        Groups: uint32(groups),
    }

    err = syscall.Bind(s, saddr)
    if err != nil {
        return nil, fmt.Errorf("bind: %s", err)
    }

    return &NetlinkListener{fd: s, sa: saddr}, nil
}

func (l *NetlinkListener) ReadMsgs() ([]syscall.NetlinkMessage, error) {
    defer func() {
        recover()
    }()

    pkt := make([]byte, 2048)

    n, err := syscall.Read(l.fd, pkt)
    if err != nil {
        return nil, fmt.Errorf("read: %s", err)
    }

    msgs, err := syscall.ParseNetlinkMessage(pkt[:n])
    if err != nil {
        return nil, fmt.Errorf("parse: %s", err)
    }

    return msgs, nil
}

func IsNewAddr(msg *syscall.NetlinkMessage) bool {
    if msg.Header.Type == syscall.RTM_NEWADDR {
        return true
    }

    return false
}

func IsDelAddr(msg *syscall.NetlinkMessage) bool {
    if msg.Header.Type == syscall.RTM_DELADDR {
        return true
    }

    return false
}

func IsRelevant(msg *syscall.IfAddrmsg) bool {
    if msg.Scope == syscall.RT_SCOPE_UNIVERSE ||
        msg.Scope == syscall.RT_SCOPE_SITE {
        return true
    }

    return false
}

if this line "groups := syscall.RTNLGRP_IPV4_IFADDR" will be changed to groups := uint32(16) it start working normaly.

  1. What did you expect to see?
    RTNLGRP_IPV4_IFADDR = 0x10 in syscall.go
  2. What did you see instead?
    RTNLGRP_IPV4_IFADDR = 0x5 in syscall.go
@bradfitz
Copy link
Contributor

bradfitz commented Apr 3, 2016

/cc @mikioh @ianlancetaylor

@bradfitz bradfitz changed the title Error in definition of constant variable RTNLGRP_IPV4_IFADDR syscall: error in definition of constant RTNLGRP_IPV4_IFADDR Apr 3, 2016
@ianlancetaylor
Copy link
Contributor

As far as I can tell, the value of RTNLGRP_IPV4_IFADDR is correct. It is a netlink multicast group. You can see it defined in linux/rtnetlink.h. You can use it by setting the 1 << RTNLGRP_IPV4_IFADDR bit in the Groups field of syscall.SockaddrNetlink. Your code doesn't work because you are treating the value as a bitmask rather than as a bit number.

You are trying to use RTMGRP_IPV4_IFADDR, a different constant that the syscall package does not define. That is the bitmask form of RTNLGRP_IPV4_IFADDR. It is considered a legacy name. New code should use the RTNLGRP constants instead, with shifts as needed.

I don't think there is anything we should change in Go here. I agree that this code is subtle, and that the names look very similar, but we are correctly defining the new API constants and I think it's reasonable for us to omit the old API. If we decide that it's important to support the old API as well, we should do it in the x/sys/unix package, not the syscall package.

I'm closing this but feel free to reopen if you disagree.

@inoyatov
Copy link
Author

inoyatov commented Apr 4, 2016

Dear @ianlancetaylor,

Thank you for correcting me. After some digging I found that many sources in C language mostly uses RTNLGRP_* constant variables through setsocketopt function. However RTMGRP_* constant variables used through socket address structure. Also I suggest to include RTMGRP_* constant variables into syscall.go since it is not legacy code or old API, but it is userspace backward compatibility issue according rtnetlink.h comment in the source.

#ifndef __KERNEL__
/* RTnetlink multicast groups - backwards compatibility for userspace */
#define RTMGRP_LINK     1
#define RTMGRP_NOTIFY       2
#define RTMGRP_NEIGH        4
#define RTMGRP_TC       8

#define RTMGRP_IPV4_IFADDR  0x10
#define RTMGRP_IPV4_MROUTE  0x20
#define RTMGRP_IPV4_ROUTE   0x40
#define RTMGRP_IPV4_RULE    0x80

#define RTMGRP_IPV6_IFADDR  0x100
#define RTMGRP_IPV6_MROUTE  0x200
#define RTMGRP_IPV6_ROUTE   0x400
#define RTMGRP_IPV6_IFINFO  0x800

#define RTMGRP_DECnet_IFADDR    0x1000
#define RTMGRP_DECnet_ROUTE     0x4000

#define RTMGRP_IPV6_PREFIX  0x20000
#endif

Morover as part of your answer 1 << RTNLGRP_IPV4_IFADDR = 32 is incorrect. While correct value for receiving IPv4 address events is 16. According iproute2 source. In utils.h line 175 it should be 1 << (RTNLGRP_IPV4_IFADDR - 1) = 16:

static inline __u32 nl_mgrp(__u32 group)
{
    if (group > 31 ) {
        fprintf(stderr, "Use setsockopt for this group %d\n", group);
        exit(-1);
    }
    return group ? (1 << (group - 1)) : 0;
}

If you see other RTNLGRP_* flags which is bit value is more than 31 should be used trough setsockopt. Does this flags are declared in syscall.go? I could not find it.

So I suggest to add RTMGRP_* constants to syscall.go or include some explanation about it in go documentation.

Here is some incorrect codes which people does not knew that it is incorrect.
https://github.com/MichielDeMey/home-controller/blob/d26d5f131856db6b34fff3dc2853c0c760c231f8/lib/monitor.go#L27
https://github.com/ghedo/moodns/blob/7d6e1117a748a41ca7d8e258603d7c45f01dbee4/src/netlink/netlink_linux.go#L43

@ianlancetaylor
Copy link
Contributor

Sorry for the incorrect suggestion.

The syscall package is frozen, so we won't be adding the RTMGRP symbols. If we do need them--it's still not clear to me that we do--they should be added to the x/sys/unix package.

@inoyatov
Copy link
Author

inoyatov commented Apr 4, 2016

Ok. It was suggestion. If it is frozen, I will understand. However how it can be possible to contribute to go lang documentation about this information? How I can join and help to improve networking part of go language? In order other will not have same problem as me.

@bradfitz
Copy link
Contributor

bradfitz commented Apr 4, 2016

@inoyatov
Copy link
Author

inoyatov commented Apr 4, 2016

Thank you.

@golang golang locked and limited conversation to collaborators Apr 5, 2017
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

4 participants