This code was trying to check for priorities greater than UINT16_MAX and
reset them, but it assigned the value to a uint16_t before it checked it,
which of course hid the problem.
Fixes the following GCC warning:
vswitchd/bridge.c:3034: warning: comparison is always false due to limited
range of data type
Reported-by: YAMAMOTO Takashi <yamamoto@valinux.co.jp>
The range of "enum" types varies from one ABI to another. If the enums
being tested in these functions happen to be 16 bits wide, then GCC may
issue a warning because, in such a case, the comparison is always true.
Using an int instead of a uint16_t avoids that particular problem and
should suppress the warning.
Fixes the following reported warnings:
lib/ofp-print.c:240: warning: comparison is always true due to limited
range of data type
lib/ofp-util.c:1973: warning: comparison is always false due to limited
range of data type
Reported-by: YAMAMOTO Takashi <yamamoto@valinux.co.jp>
The ctype functions often need casts to be fully C standards compliant.
Here's the full explanation that I used to post to comp.lang.c from time
to time when the issue came up:
With the to*() and is*() functions, you should be careful to cast
`char' arguments to `unsigned char' before calling them. Type `char'
may be signed or unsigned, depending on your compiler or its
configuration. If `char' is signed, then some characters have
negative values; however, the arguments to is*() and to*() functions
must be nonnegative (or EOF). Casting to `unsigned char' fixes this
problem by forcing the character to the corresponding positive value.
This fixes the following warnings from some version of GCC:
lib/ofp-parse.c:828: warning: array subscript has type 'char'
lib/ofp-print.c:617: warning: array subscript has type 'char'
Reported-by: YAMAMOTO Takashi <yamamoto@valinux.co.jp>
The bonding code only needs to know whether a given slave may be
enabled, and whether LACP has been negotiated on the bond. Instead
of passing in the LACP handle and letting the bond query this
information. This patch passes in the information directly.
For BM_STABLE bonds, instead of choosing the sort key in the
qsort() comparator, this patch makes it a configuration setting of
each slave. This will help wrest LACP out of the bonding code
further in future patches.
Before this patch, the bonding code had taken over responsibility
for running the LACP module. However, the bonding code only needs
the LACP module for some basic status queries. LACP and bonding
are actually logically parallel modules and do not really have a
parent child relationship. Furthermore, we need to be able to run
LACP on non-bonded interfaces which the existing approach
prevented. This patch gives control of the LACP module back to the
bridge.
The enabled flag in the LACP module was only used to set the
Collecting and Distributing flags in the LACP protocol. It was
intended to be set by the bonding code to mimic its enabled flag.
The spec is relatively vague on the precise meaning of these flags,
and most implementations do something completely different with
them. For these reasons, it seems acceptable to remove the enabled
flag for the sake of simplicity. A slave is now Collecting and
Distributing if it is attached, or LACP couldn't be negotiated.
Slave registration should go through the normal slave enabling
facilities instead of doing it by hand. Before this patch, newly
created slaves would have no tag associated with them.
Furthermore, any further changes to how slaves are enabled would
not be picked up by the registration code.
When all flows in a bond are revalidated, stale bond_entry's can
cause incorrect load balancing. These issues will naturally
resolve themselves overtime. However, it's better to deal with
them immediately.
If LACP causes the return of bond_is_tcp_hash to change for
whatever reason, all flows should be revalidated because they will
have a different hash result.
Changes in the bonding mode can cause drastic changes in flow
assignments to slaves. This commit causes all flows in a bridge
to be revalidated when bond_reconfigure() changes its bonding mode.
This approach is a bit aggressive, but bond reconfiguration
shouldn't happen often.
-Werror is useful for development, but it screws up configure because it's
impossible to guess what new warnings compilers will add in the future.
This commit adds a new configure option to add CFLAGS after the configure
checks are done.
The use of AC_CONFIG_COMMANDS_PRE is based on Eric Blake's suggestion on
the autoconf mailing list: "AC_CONFIG_COMMANDS_PRE probably fits the bill
as the ideal macro to use for guaranteeing that you inject your shell code
at the last possible moment."
Requested-by: Andrew Evans <aevans@nicira.com>
Deciding what delete operations to issue on garbage-collected tables has
been a bit of a difficult issue for ovs-vsctl. When garbage collection was
introduced in commit c5f341a "ovsdb: Implement garbage collection",
ovs-vsctl did not issue any deletions for these tables at all. As a side
effect, ovs-vsctl did not notice that records were going to be deleted.
That meant that when multiple commands were issued in one ovs-vsctl run,
ovs-vsctl could get confused by apparent duplicate records that did not
in fact exist. Commit 28a14bf "ovs-vsctl: Back out garbage collection
changes" fixed the problem by putting all of the explicit deletions back
into ovs-vsctl.
However, adding these explicit deletions had the price that it then became
(again) impossible to use ovs-vsctl commands to delete duplicates, for
example to use "ovs-vsctl del-br" to delete a bridge that points to the
same Port records that some other Bridge record also does. This commit
makes that possible again, by implementing a compromise:
* Internally, ovs-vsctl deletes the records that it believes should be
deleted.
* ovsdb-idl suppresses the deletions when it makes the RPC call into
the database server.
Bug #5358.
Reported-by: Henrik Amren <henrik@nicira.com>
This command was removed in commit 9b45d7f5d (ofproto: Get rid of archaic
"switch status" OpenFlow extension) but I didn't notice that ovs-bugtool
uses that command and forgot to remove it at the time.
Bug #5360.
Reported-by: Michael Mao <mmao@nicira.com>
Reported-by: Keith Amidon <keith@nicira.com>
netdev_linux_get_stats() calls into netdev_vport_get_stats(), which in
turn attempts a transaction on genl_sock. If the kernel module isn't
loaded, then genl_sock won't be there, and in any case there's nothing that
guarantees that it's been initialized yet.
This fixes the problem by ensuring that dpif_linux was initialized properly
before attempting a transaction on genl_sock.
Reported-by: Aaron Rosen <arosen@clemson.edu>
Commit 76c308b50d "netdev-linux: Support 'send' for netdevs opened with
NETDEV_ETH_TYPE_NONE" broke sending packets to tap devices. Sending a
packet to a tap device with an AF_PACKET socket causes that packet to be
looped back to be received on the tap device again, which obviously isn't
useful.
Obviously correct code is easier on everyone. As the C FAQ says:
20.15c: How can I swap two values without using a temporary?
A: The standard hoary old assembly language programmer's trick is:
a ^= b;
b ^= a;
a ^= b;
But this sort of code has little place in modern, HLL
programming. Temporary variables are essentially free,
and the idiomatic code using three assignments, namely
int t = a;
a = b;
b = t;
is not only clearer to the human reader, it is more likely to be
recognized by the compiler and turned into the most-efficient
code (e.g. using a swap instruction, if available). The latter
code is obviously also amenable to use with pointers and
floating-point values, unlike the XOR trick. See also questions
3.3b and 10.3.
By omitting columns that ovs-vswitchd does not use at all, and omitting
alerts for columns that ovs-vswitchd writes to but does not read, we can
save CPU time and bandwidth.
The existing ovsdb_idl_txn_commit() drops any writes that don't change a
column's value from what was last reported by the database. But this isn't
a valid optimization, because it breaks the atomicity of transactions.
Suppose columns A and B initially have values 1 and 2. Client 1 writes
value 1 to both columns in one transaction. Client 2 writes value 2 to
both columns in another transaction. The only possible valid results for
any serial ordering of transactions are 1,1 or 2,2. But if both clients
drop writes to columns that they have not modified, then 2,1 also becomes
possible (because client 1 just writes to B and client 2 just writes to A).
However, for write-only columns we can optimize this out because the IDL
can assume it is the only client writing to a column.
Found by inspection.
netdev_rx_handler_register() changed the type of the skb argument to the
callback function as well as the return type. Special-case
netdev_frame_hook() to do the right thing on 2.6.39 and later.
Signed-off-by: Andrew Evans <aevans@nicira.com>
Acked-by: Jesse Gross <jesse@nicira.com>
If the last receive time for a remote MP was before the last fault
check, the CFM code would not declare a fault. This is, of course,
exactly the wrong response.
Bug #5303.
CFM configuration requires the ofproto_run function to have been
executed at least once in order to guarantee that the relevant
ports exist.
Bug #5303.
Before this patch the kernel chose the lowest available number for
newly created datapath ports. This patch moves the port number
choosing responsibility to user space, and implements a least
recently used port number queue in an attempt to avoid reuse.
Bug #2140.
When the bonding library encounters a flow it hasn't seen before,
it assigns it to the active slave and waits for load balancing to
move it to a more appropriate place. This commit causes it to
first attempt a random slave.
Until now, if two copies of one OVS daemon started up at the same time,
then due to races in pidfile creation it was possible for both of them to
start successfully, instead of just one. This was made worse when a
previous copy of the daemon had died abruptly, leaving a stale pidfile.
This commit implements a new pidfile creation and removal protocol that I
believe closes these races. Now, a pidfile is asserted with "link" instead
of "rename", which prevents the race on creation, and a stale pidfile may
only be deleted by a process after it has taken a lock on it.
This may solve mysterious problems seen occasionally on vswitch restart.
I'm still puzzled by these problems, however, because I don't see anything
in our tests cases that would actually cause two copies of a daemon to
start at the same time, which as far as I can see is a necessary
precondition for the problem.
Until now, it has been the responsibility of an individual daemon to call
die_if_already_running() at an appropriate time. A long time ago, this
had to happen *before* daemonizing, because once the process daemonized
itself there was no way to report failure to the process that originally
started the daemon. With the introduction of daemonize_start(), this is
now possible, but we haven't been taking advantage of it.
Therefore, this commit integrates the die_if_already_running() call into
daemonize_start() and deletes the calls to it from individual daemons.
This matches what xmalloc() does. It will be handled better by a monitor
process (created with --monitor), which will restart the child instead of
exiting.
ovs-xapi-sync is supposed to always keep external-ids:iface-id up to date,
but in fact it would only set it when an interface initially appeared. If
the interface quickly disappeared and reappeared, then it failed to notice
that iface-id had changed or disappeared. This happens in practice on
Citrix XenServer, where VM "tap" devices often disappear and then reappear
almost immediately during VM boot. This commit fixes the problem.
This also fixes the similar problem for external-ids:bridge-id in Bridge
records. Bridges aren't ordinarily destroyed and re-created quickly, so
this problem might never have manifested in practice for bridges.
Many thanks to Reid Price <reid@nicira.com> for identifying the problem
and supplying an initial fix.
Bug #5239.
Reported-by: Henrik Amren <henrik@nicira.com>
