| Message ID | 23806aac6db3baf7e2cdab4c62d6e3468ce6b4dc.1471340849.git.mprivozn@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 16 August 2016 at 10:47, Michal Privoznik <mprivozn@redhat.com> wrote: > In c5dff280 we tried to make us understand netlink messages more. > So we've added a code that does some translation. However, the > code assumed linux-headers to be at least version 4.4 of it > because most of the symbols there (if not all of them) were added > in just that release. This, however, breaks build on systems with > older versions of the package. > > Signed-off-by: Michal Privoznik <mprivozn@redhat.com> > --- > linux-user/syscall.c | 86 ++++++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 86 insertions(+) > u32 = NLA_DATA(nlattr); > *u32 = tswap32(*u32); > break; > /* uint64_t */ > +#ifdef IFLA_BR_HELLO_TIMER > case IFLA_BR_HELLO_TIMER: > +#endif > +#ifdef IFLA_BR_TCN_TIMER > case IFLA_BR_TCN_TIMER: > +#endif > +#ifdef IFLA_BR_GC_TIMER > case IFLA_BR_GC_TIMER: > +#endif > +#ifdef IFLA_BR_TOPOLOGY_CHANGE_TIMER > case IFLA_BR_TOPOLOGY_CHANGE_TIMER: > +#endif > +#ifdef IFLA_BR_MCAST_LAST_MEMBER_INTVL > case IFLA_BR_MCAST_LAST_MEMBER_INTVL: > +#endif > +#ifdef IFLA_BR_MCAST_MEMBERSHIP_INTVL > case IFLA_BR_MCAST_MEMBERSHIP_INTVL: > +#endif > +#ifdef IFLA_BR_MCAST_QUERIER_INTVL > case IFLA_BR_MCAST_QUERIER_INTVL: > +#endif > +#ifdef IFLA_BR_MCAST_QUERY_INTVL > case IFLA_BR_MCAST_QUERY_INTVL: > +#endif > +#ifdef IFLA_BR_MCAST_QUERY_RESPONSE_INTVL > case IFLA_BR_MCAST_QUERY_RESPONSE_INTVL: > +#endif > +#ifdef IFLA_BR_MCAST_STARTUP_QUERY_INTVL > case IFLA_BR_MCAST_STARTUP_QUERY_INTVL: > +#endif > u64 = NLA_DATA(nlattr); > *u64 = tswap64(*u64); > break; > /* ifla_bridge_id: uin8_t[] */ > +#ifdef IFLA_BR_ROOT_ID > case IFLA_BR_ROOT_ID: > +#endif > +#ifdef IFLA_BR_BRIDGE_ID > case IFLA_BR_BRIDGE_ID: > +#endif > break; Aren't there complaints about unreachable code if the defines are all undefined ? thanks -- PMM
On 16.08.2016 12:04, Peter Maydell wrote: > On 16 August 2016 at 10:47, Michal Privoznik <mprivozn@redhat.com> wrote: >> In c5dff280 we tried to make us understand netlink messages more. >> So we've added a code that does some translation. However, the >> code assumed linux-headers to be at least version 4.4 of it >> because most of the symbols there (if not all of them) were added >> in just that release. This, however, breaks build on systems with >> older versions of the package. >> >> Signed-off-by: Michal Privoznik <mprivozn@redhat.com> >> --- >> linux-user/syscall.c | 86 ++++++++++++++++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 86 insertions(+) > > >> u32 = NLA_DATA(nlattr); >> *u32 = tswap32(*u32); >> break; >> /* uint64_t */ >> +#ifdef IFLA_BR_HELLO_TIMER >> case IFLA_BR_HELLO_TIMER: >> +#endif >> +#ifdef IFLA_BR_TCN_TIMER >> case IFLA_BR_TCN_TIMER: >> +#endif >> +#ifdef IFLA_BR_GC_TIMER >> case IFLA_BR_GC_TIMER: >> +#endif >> +#ifdef IFLA_BR_TOPOLOGY_CHANGE_TIMER >> case IFLA_BR_TOPOLOGY_CHANGE_TIMER: >> +#endif >> +#ifdef IFLA_BR_MCAST_LAST_MEMBER_INTVL >> case IFLA_BR_MCAST_LAST_MEMBER_INTVL: >> +#endif >> +#ifdef IFLA_BR_MCAST_MEMBERSHIP_INTVL >> case IFLA_BR_MCAST_MEMBERSHIP_INTVL: >> +#endif >> +#ifdef IFLA_BR_MCAST_QUERIER_INTVL >> case IFLA_BR_MCAST_QUERIER_INTVL: >> +#endif >> +#ifdef IFLA_BR_MCAST_QUERY_INTVL >> case IFLA_BR_MCAST_QUERY_INTVL: >> +#endif >> +#ifdef IFLA_BR_MCAST_QUERY_RESPONSE_INTVL >> case IFLA_BR_MCAST_QUERY_RESPONSE_INTVL: >> +#endif >> +#ifdef IFLA_BR_MCAST_STARTUP_QUERY_INTVL >> case IFLA_BR_MCAST_STARTUP_QUERY_INTVL: >> +#endif >> u64 = NLA_DATA(nlattr); >> *u64 = tswap64(*u64); >> break; >> /* ifla_bridge_id: uin8_t[] */ >> +#ifdef IFLA_BR_ROOT_ID >> case IFLA_BR_ROOT_ID: >> +#endif >> +#ifdef IFLA_BR_BRIDGE_ID >> case IFLA_BR_BRIDGE_ID: >> +#endif >> break; > > Aren't there complaints about unreachable code if the > defines are all undefined ? Yeah, I was worried about that too, but no. My compiler must be wise enough to realize what is going on. I see no error, nor warning. Michal
On 16 August 2016 at 10:47, Michal Privoznik <mprivozn@redhat.com> wrote: > In c5dff280 we tried to make us understand netlink messages more. > So we've added a code that does some translation. However, the > code assumed linux-headers to be at least version 4.4 of it > because most of the symbols there (if not all of them) were added > in just that release. This, however, breaks build on systems with > older versions of the package. > > Signed-off-by: Michal Privoznik <mprivozn@redhat.com> > --- > linux-user/syscall.c | 86 ++++++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 86 insertions(+) > Applied to master, thanks. -- PMM
Le 16/08/2016 à 11:47, Michal Privoznik a écrit : > In c5dff280 we tried to make us understand netlink messages more. > So we've added a code that does some translation. However, the > code assumed linux-headers to be at least version 4.4 of it > because most of the symbols there (if not all of them) were added > in just that release. This, however, breaks build on systems with > older versions of the package. > > Signed-off-by: Michal Privoznik <mprivozn@redhat.com> Are you sure this "#ifdef" are correct while all these symbols are enums not #define? Or do I miss something? Thanks, Laurent
On 16 August 2016 at 17:41, Laurent Vivier <laurent@vivier.eu> wrote: > Le 16/08/2016 à 11:47, Michal Privoznik a écrit : >> In c5dff280 we tried to make us understand netlink messages more. >> So we've added a code that does some translation. However, the >> code assumed linux-headers to be at least version 4.4 of it >> because most of the symbols there (if not all of them) were added >> in just that release. This, however, breaks build on systems with >> older versions of the package. >> >> Signed-off-by: Michal Privoznik <mprivozn@redhat.com> > > Are you sure this "#ifdef" are correct while all these symbols are enums > not #define? > > Or do I miss something? I think you're right (some of the IFLA constants have #defines in the headers I have but not the IFLA_BR_ or IFLA_BRPORT_ ones). Unfortunately I've just committed this patch and tagged rc3, so we'll have to fix this up for rc4 :-( The best approach I can think of is to add something at the top of syscall.c that does: #if IFLA_BR_MAX < 9 #define IFLA_BR_GROUP_FWD_MASK 9 #endif #if IFLA_BR_MAX < 10 #define IFLA_BR_ROOT_ID 10 #endif etc etc and then we can unconditionally use the symbols in the switches. Anybody got a better idea? Michal, can you produce a patch? thanks -- PMM
On 16.08.2016 18:51, Peter Maydell wrote: > On 16 August 2016 at 17:41, Laurent Vivier <laurent@vivier.eu> wrote: >> Le 16/08/2016 à 11:47, Michal Privoznik a écrit : >>> In c5dff280 we tried to make us understand netlink messages more. >>> So we've added a code that does some translation. However, the >>> code assumed linux-headers to be at least version 4.4 of it >>> because most of the symbols there (if not all of them) were added >>> in just that release. This, however, breaks build on systems with >>> older versions of the package. >>> >>> Signed-off-by: Michal Privoznik <mprivozn@redhat.com> >> >> Are you sure this "#ifdef" are correct while all these symbols are enums >> not #define? >> >> Or do I miss something? > > I think you're right (some of the IFLA constants have #defines in > the headers I have but not the IFLA_BR_ or IFLA_BRPORT_ ones). > Unfortunately I've just committed this patch and tagged rc3, so we'll > have to fix this up for rc4 :-( > > The best approach I can think of is to add something at the > top of syscall.c that does: > #if IFLA_BR_MAX < 9 > #define IFLA_BR_GROUP_FWD_MASK 9 > #endif > #if IFLA_BR_MAX < 10 > #define IFLA_BR_ROOT_ID 10 > #endif > etc etc > > and then we can unconditionally use the symbols in the switches. > > Anybody got a better idea? > > Michal, can you produce a patch? Oh right. Sorry guys, I don't know what I've been thinking producing such a silly patch in the first place. Sorry for the noise. I'll send fixup! Michal
Le 16/08/2016 à 18:51, Peter Maydell a écrit : > On 16 August 2016 at 17:41, Laurent Vivier <laurent@vivier.eu> wrote: >> Le 16/08/2016 à 11:47, Michal Privoznik a écrit : >>> In c5dff280 we tried to make us understand netlink messages more. >>> So we've added a code that does some translation. However, the >>> code assumed linux-headers to be at least version 4.4 of it >>> because most of the symbols there (if not all of them) were added >>> in just that release. This, however, breaks build on systems with >>> older versions of the package. >>> >>> Signed-off-by: Michal Privoznik <mprivozn@redhat.com> >> >> Are you sure this "#ifdef" are correct while all these symbols are enums >> not #define? >> >> Or do I miss something? > > I think you're right (some of the IFLA constants have #defines in > the headers I have but not the IFLA_BR_ or IFLA_BRPORT_ ones). > Unfortunately I've just committed this patch and tagged rc3, so we'll > have to fix this up for rc4 :-( > > The best approach I can think of is to add something at the > top of syscall.c that does: > #if IFLA_BR_MAX < 9 > #define IFLA_BR_GROUP_FWD_MASK 9 > #endif > #if IFLA_BR_MAX < 10 > #define IFLA_BR_ROOT_ID 10 > #endif > etc etc > > and then we can unconditionally use the symbols in the switches. > > Anybody got a better idea? Perhaps we can define our own enum with all the known values at the moment, something like QEMU_IFLA_BR_XXX? Thanks, Laurent
On 16 August 2016 at 18:28, Laurent Vivier <laurent@vivier.eu> wrote: > > > Le 16/08/2016 à 18:51, Peter Maydell a écrit : >> The best approach I can think of is to add something at the >> top of syscall.c that does: >> #if IFLA_BR_MAX < 9 >> #define IFLA_BR_GROUP_FWD_MASK 9 >> #endif >> #if IFLA_BR_MAX < 10 >> #define IFLA_BR_ROOT_ID 10 >> #endif >> etc etc >> >> and then we can unconditionally use the symbols in the switches. >> >> Anybody got a better idea? > > Perhaps we can define our own enum with all the known values at the > moment, something like QEMU_IFLA_BR_XXX? Yes, that's probably better (we usually call those TARGET_FOO rather than QEMU_FOO). Or we could follow what linux-user/linux_loop.h does, though I don't much like that approach. thanks -- PMM
Le 16/08/2016 à 19:31, Peter Maydell a écrit : > On 16 August 2016 at 18:28, Laurent Vivier <laurent@vivier.eu> wrote: >> >> >> Le 16/08/2016 à 18:51, Peter Maydell a écrit : >>> The best approach I can think of is to add something at the >>> top of syscall.c that does: >>> #if IFLA_BR_MAX < 9 >>> #define IFLA_BR_GROUP_FWD_MASK 9 >>> #endif >>> #if IFLA_BR_MAX < 10 >>> #define IFLA_BR_ROOT_ID 10 >>> #endif >>> etc etc >>> >>> and then we can unconditionally use the symbols in the switches. >>> >>> Anybody got a better idea? >> >> Perhaps we can define our own enum with all the known values at the >> moment, something like QEMU_IFLA_BR_XXX? > > Yes, that's probably better (we usually call those TARGET_FOO > rather than QEMU_FOO). Or we could follow what linux-user/linux_loop.h > does, though I don't much like that approach. I think we should not use TARGET_ because we want to use them with values coming from the host and not only from the target. Thanks, Laurent
diff --git a/linux-user/syscall.c b/linux-user/syscall.c index 833f853..7edfe4a 100644 --- a/linux-user/syscall.c +++ b/linux-user/syscall.c @@ -1783,30 +1783,58 @@ static abi_long host_to_target_data_bridge_nlattr(struct nlattr *nlattr, uint64_t *u64; switch (nlattr->nla_type) { +#ifdef IFLA_BR_FDB_FLUSH /* no data */ case IFLA_BR_FDB_FLUSH: break; +#endif +#ifdef IFLA_BR_GROUP_ADDR /* binary */ case IFLA_BR_GROUP_ADDR: break; +#endif /* uint8_t */ case IFLA_BR_VLAN_FILTERING: +#ifdef IFLA_BR_TOPOLOGY_CHANGE case IFLA_BR_TOPOLOGY_CHANGE: +#endif +#ifdef IFLA_BR_TOPOLOGY_CHANGE_DETECTED case IFLA_BR_TOPOLOGY_CHANGE_DETECTED: +#endif +#ifdef IFLA_BR_MCAST_ROUTER case IFLA_BR_MCAST_ROUTER: +#endif +#ifdef IFLA_BR_MCAST_SNOOPING case IFLA_BR_MCAST_SNOOPING: +#endif +#ifdef IFLA_BR_MCAST_QUERY_USE_IFADDR case IFLA_BR_MCAST_QUERY_USE_IFADDR: +#endif +#ifdef IFLA_BR_MCAST_QUERIER case IFLA_BR_MCAST_QUERIER: +#endif +#ifdef IFLA_BR_NF_CALL_IPTABLES case IFLA_BR_NF_CALL_IPTABLES: +#endif +#ifdef IFLA_BR_NF_CALL_IP6TABLES case IFLA_BR_NF_CALL_IP6TABLES: +#endif +#ifdef IFLA_BR_NF_CALL_ARPTABLES case IFLA_BR_NF_CALL_ARPTABLES: +#endif break; /* uint16_t */ case IFLA_BR_PRIORITY: case IFLA_BR_VLAN_PROTOCOL: +#ifdef IFLA_BR_GROUP_FWD_MASK case IFLA_BR_GROUP_FWD_MASK: +#endif +#ifdef IFLA_BR_ROOT_PORT case IFLA_BR_ROOT_PORT: +#endif +#ifdef IFLA_BR_VLAN_DEFAULT_PVID case IFLA_BR_VLAN_DEFAULT_PVID: +#endif u16 = NLA_DATA(nlattr); *u16 = tswap16(*u16); break; @@ -1816,31 +1844,65 @@ static abi_long host_to_target_data_bridge_nlattr(struct nlattr *nlattr, case IFLA_BR_MAX_AGE: case IFLA_BR_AGEING_TIME: case IFLA_BR_STP_STATE: +#ifdef IFLA_BR_ROOT_PATH_COST case IFLA_BR_ROOT_PATH_COST: +#endif +#ifdef IFLA_BR_MCAST_HASH_ELASTICITY case IFLA_BR_MCAST_HASH_ELASTICITY: +#endif +#ifdef IFLA_BR_MCAST_HASH_MAX case IFLA_BR_MCAST_HASH_MAX: +#endif +#ifdef IFLA_BR_MCAST_LAST_MEMBER_CNT case IFLA_BR_MCAST_LAST_MEMBER_CNT: +#endif +#ifdef IFLA_BR_MCAST_STARTUP_QUERY_CNT case IFLA_BR_MCAST_STARTUP_QUERY_CNT: +#endif u32 = NLA_DATA(nlattr); *u32 = tswap32(*u32); break; /* uint64_t */ +#ifdef IFLA_BR_HELLO_TIMER case IFLA_BR_HELLO_TIMER: +#endif +#ifdef IFLA_BR_TCN_TIMER case IFLA_BR_TCN_TIMER: +#endif +#ifdef IFLA_BR_GC_TIMER case IFLA_BR_GC_TIMER: +#endif +#ifdef IFLA_BR_TOPOLOGY_CHANGE_TIMER case IFLA_BR_TOPOLOGY_CHANGE_TIMER: +#endif +#ifdef IFLA_BR_MCAST_LAST_MEMBER_INTVL case IFLA_BR_MCAST_LAST_MEMBER_INTVL: +#endif +#ifdef IFLA_BR_MCAST_MEMBERSHIP_INTVL case IFLA_BR_MCAST_MEMBERSHIP_INTVL: +#endif +#ifdef IFLA_BR_MCAST_QUERIER_INTVL case IFLA_BR_MCAST_QUERIER_INTVL: +#endif +#ifdef IFLA_BR_MCAST_QUERY_INTVL case IFLA_BR_MCAST_QUERY_INTVL: +#endif +#ifdef IFLA_BR_MCAST_QUERY_RESPONSE_INTVL case IFLA_BR_MCAST_QUERY_RESPONSE_INTVL: +#endif +#ifdef IFLA_BR_MCAST_STARTUP_QUERY_INTVL case IFLA_BR_MCAST_STARTUP_QUERY_INTVL: +#endif u64 = NLA_DATA(nlattr); *u64 = tswap64(*u64); break; /* ifla_bridge_id: uin8_t[] */ +#ifdef IFLA_BR_ROOT_ID case IFLA_BR_ROOT_ID: +#endif +#ifdef IFLA_BR_BRIDGE_ID case IFLA_BR_BRIDGE_ID: +#endif break; default: gemu_log("Unknown IFLA_BR type %d\n", nlattr->nla_type); @@ -1868,16 +1930,30 @@ static abi_long host_to_target_slave_data_bridge_nlattr(struct nlattr *nlattr, case IFLA_BRPORT_PROXYARP: case IFLA_BRPORT_LEARNING_SYNC: case IFLA_BRPORT_PROXYARP_WIFI: +#ifdef IFLA_BRPORT_TOPOLOGY_CHANGE_ACK case IFLA_BRPORT_TOPOLOGY_CHANGE_ACK: +#endif +#ifdef IFLA_BRPORT_CONFIG_PENDING case IFLA_BRPORT_CONFIG_PENDING: +#endif +#ifdef IFLA_BRPORT_MULTICAST_ROUTER case IFLA_BRPORT_MULTICAST_ROUTER: +#endif break; /* uint16_t */ case IFLA_BRPORT_PRIORITY: +#ifdef IFLA_BRPORT_DESIGNATED_PORT case IFLA_BRPORT_DESIGNATED_PORT: +#endif +#ifdef IFLA_BRPORT_DESIGNATED_COST case IFLA_BRPORT_DESIGNATED_COST: +#endif +#ifdef IFLA_BRPORT_ID case IFLA_BRPORT_ID: +#endif +#ifdef IFLA_BRPORT_NO case IFLA_BRPORT_NO: +#endif u16 = NLA_DATA(nlattr); *u16 = tswap16(*u16); break; @@ -1887,15 +1963,25 @@ static abi_long host_to_target_slave_data_bridge_nlattr(struct nlattr *nlattr, *u32 = tswap32(*u32); break; /* uint64_t */ +#ifdef IFLA_BRPORT_MESSAGE_AGE_TIMER case IFLA_BRPORT_MESSAGE_AGE_TIMER: +#endif +#ifdef IFLA_BRPORT_FORWARD_DELAY_TIMER case IFLA_BRPORT_FORWARD_DELAY_TIMER: +#endif +#ifdef IFLA_BRPORT_HOLD_TIMER case IFLA_BRPORT_HOLD_TIMER: +#endif u64 = NLA_DATA(nlattr); *u64 = tswap64(*u64); break; /* ifla_bridge_id: uint8_t[] */ +#ifdef IFLA_BRPORT_ROOT_ID case IFLA_BRPORT_ROOT_ID: +#endif +#ifdef IFLA_BRPORT_BRIDGE_ID case IFLA_BRPORT_BRIDGE_ID: +#endif break; default: gemu_log("Unknown IFLA_BRPORT type %d\n", nlattr->nla_type);
In c5dff280 we tried to make us understand netlink messages more. So we've added a code that does some translation. However, the code assumed linux-headers to be at least version 4.4 of it because most of the symbols there (if not all of them) were added in just that release. This, however, breaks build on systems with older versions of the package. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- linux-user/syscall.c | 86 ++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 86 insertions(+)