aboutgitcodebugslistschat
Commit message (Collapse)AuthorAgeFilesLines
* pasta: More general way of starting spawned shell as a login shellDavid Gibson2022-10-151-12/+20
| | | | | | | | | | | | | | When invoked so as to spawn a shell, pasta checks explicitly for the shell being bash and if so, adds a "-l" option to make it a login shell. This is not ideal, since this is a bash specific option and requires pasta to know about specific shell variants. There's a general convention for starting a login shell, which is to prepend a "-" to argv[0]. Use this approach instead, so we don't need bash specific logic. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* test: Move slower tests to end of test runDavid Gibson2022-10-151-10/+10
| | | | | | | | | | The distro and performance tests are by far the slowest part of the passt testsuite. Move them to the end of the testsuite run, so that it's easier to do a quick test during development by letting the other tests run then interrupting the test runner. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* log.h: Avoid unnecessary GNU extension for token pastingStefano Brivio2022-10-151-2/+2
| | | | | | | | | | | | clang says: ./log.h:23:18: warning: token pasting of ',' and __VA_ARGS__ is a GNU extension [-Wgnu-zero-variadic-macro-arguments] We need token pasting here just because of the 'format' in trace(): drop it. Suggested-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* util.h: Add missing gcc pragma push before pragma popStefano Brivio2022-10-151-0/+1
| | | | | | | | | While building with clang: ./util.h:176:24: warning: pragma diagnostic pop could not pop, no matching push [-Wunknown-pragmas] Signed-off-by: Stefano Brivio <sbrivio@redhat.com> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
* icmp: Set sin6_scope_id for outbound ICMPv6 echo requestsStefano Brivio2022-10-151-0/+1
| | | | | | | | | | | If we ping a link-local address, we need to pass this to sendto(), as it will obviously fail with -EINVAL otherwise. If we ping other addresses, it's probably a good idea anyway to specify the configured outbound interface here. Signed-off-by: Stefano Brivio <sbrivio@redhat.com> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
* conf: Drop excess colons in usage for DHCP and DNS optionsStefano Brivio2022-10-151-4/+4
| | | | | Signed-off-by: Stefano Brivio <sbrivio@redhat.com> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
* netlink: Disable duplicate address detection for configured IPv6 addressStefano Brivio2022-10-151-0/+3
| | | | | | | | | | | | | | | | | | With default options, when we pass --config-net, the IPv6 address is actually going to be recycled from the init namespace, so it is in fact duplicated, but duplicate address detection has no way to find out. With a different configured address, that's not the case, but anyway duplicate address detection will be unable to see this. In both cases, we're wasting time for nothing. Pass the IFA_F_NODAD flag as we configure globally scoped IPv6 addresses via netlink. Signed-off-by: Stefano Brivio <sbrivio@redhat.com> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
* Don't create 'tap' socket for ports that are bound to loopback onlyStefano Brivio2022-10-152-69/+117
| | | | | | | | | | | | | | | | | | | | | If the user specifies an explicit loopback address for a port binding, we're going to use that address for the 'tap' socket, and the same exact address for the 'spliced' socket (because those are, by definition, only bound to loopback addresses). This means that the second binding will fail, and, unexpectedly, the port is forwarded, but via tap device, which means the source address in the namespace won't be a loopback address. Make it explicit under which conditions we're creating which kind of socket, by refactoring tcp_sock_init() into two separate functions for IPv4 and IPv6 and gathering those conditions at the beginning. Also, don't create spliced sockets if the user specifies explicitly a non-loopback address, those are harmless but not desired either. Fixes: 3c6ae625101a ("conf, tcp, udp: Allow address specification for forwarded ports") Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* tcp, tcp_splice: Fix port remapping for inbound, spliced connectionsStefano Brivio2022-10-153-11/+18
| | | | | | | | | | | | | | | | | | | | | | | In pasta mode, when we receive a new inbound connection, we need to select a socket that was created in the namespace to proceed and connect() it to its final destination. The existing condition might pick a wrong socket, though, if the destination port is remapped, because we'll check the bitmap of inbound ports using the remapped port (stored in the epoll reference) as index, and not the original port. Instead of using the port bitmap for this purpose, store this information in the epoll reference itself, by adding a new 'outbound' bit, that's set if the listening socket was created the namespace, and unset otherwise. Then, use this bit to pick a socket on the right side. Suggested-by: David Gibson <david@gibson.dropbear.id.au> Fixes: 33482d5bf293 ("passt: Add PASTA mode, major rework") Signed-off-by: Stefano Brivio <sbrivio@redhat.com> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
* tcp, tcp_splice: Adjust comments to current meaning of inbound and outboundStefano Brivio2022-10-152-2/+2
| | | | | | | | | | | | | | | | | | | | For tcp_sock_init_ns(), "inbound" connections used to be the ones being established toward any listening socket we create, as opposed to sockets we connect(). Similarly, tcp_splice_new() used to handle "inbound" connections in the sense that they originated from listening sockets, and they would in turn cause a connect() on an "outbound" socket. Since commit 1128fa03fe73 ("Improve types and names for port forwarding configuration"), though, inbound connections are more broadly defined as the ones directed to guest or namepsace, and outbound the ones originating from there. Update comments for those two functions. Signed-off-by: Stefano Brivio <sbrivio@redhat.com> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
* udp: Fix port and address checks for DNS forwarderStefano Brivio2022-10-151-3/+3
| | | | | | | | | | | | | | | | | | | First off, as we swap endianness for source ports in udp_fill_data_v{4,6}(), we want host endianness, not network endianness. It doesn't actually matter if we use htons() or ntohs() here, but the current version is confusing. In the IPv4 path, when we remap DNS answers, we already swapped the endianness as needed for the source port: don't swap it again, otherwise we'll not map DNS answers for IPv4. In the IPv6 path, when we remap DNS answers, we want to check that they came from our upstream DNS server, not the one configured via --dns-forward (which doesn't even need to exist for this functionality to work). Signed-off-by: Stefano Brivio <sbrivio@redhat.com> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
* tap: Don't check sequence counts when adding packets to poolStefano Brivio2022-10-151-6/+6
| | | | | | | | | | | This is a minor optimisation possibility I spotted while trying to debug a hang in tap4_handler(): if we run out of space for packet sequences, it's fine to add packets to an existing per-sequence pool. We should check the count of packet sequences only once we realise that we actually need a new packet sequence. Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* packet: Fix off-by-one in packet_get_do() sanity checksStefano Brivio2022-10-151-1/+1
| | | | | | | | | | | | | An n-sized pool, or a pool with n entries, doesn't include index n, only up to n - 1. I'm not entirely sure this sanity check actually covers any practical case, but I spotted this while debugging a hang in tap4_handler() (possibly due to malformed sequence entries from qemu). Signed-off-by: Stefano Brivio <sbrivio@redhat.com> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
* conf: Report usage for --no-netns-quitStefano Brivio2022-10-151-0/+2
| | | | | | Fixes: 745a9ba4284c ("pasta: By default, quit if filesystem-bound net namespace goes away") Signed-off-by: Stefano Brivio <sbrivio@redhat.com> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
* conf, tcp, udp: Allow specification of interface to bind toStefano Brivio2022-10-159-47/+88
| | | | | | | | | | | | | | | | Since kernel version 5.7, commit c427bfec18f2 ("net: core: enable SO_BINDTODEVICE for non-root users"), we can bind sockets to interfaces, if they haven't been bound yet (as in bind()). Introduce an optional interface specification for forwarded ports, prefixed by %, that can be passed together with an address. Reported use case: running local services that use ports we want to have externally forwarded: https://github.com/containers/podman/issues/14425 Signed-off-by: Stefano Brivio <sbrivio@redhat.com> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
* conf, tap: Add option to quit once the client closes the connectionStefano Brivio2022-10-154-1/+27
| | | | | | | | This is practical to avoid explicit lifecycle management in users, e.g. libvirtd, and is trivial to implement. Signed-off-by: Stefano Brivio <sbrivio@redhat.com> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
* util: Check return value of lseek() while reading bound ports from procfsStefano Brivio2022-10-151-3/+7
| | | | | | | | | Coverity now noticed we're checking most lseek() return values, but not this. Not really relevant, but it doesn't hurt to check we can actually seek before reading lines. Signed-off-by: Stefano Brivio <sbrivio@redhat.com> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
* conf, log, Makefile: Add versioning informationStefano Brivio2022-10-156-3/+26
| | | | | | | Add a --version option displaying that, and also include this information in the log files. Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* log: Add missing function comment for trace_init()Stefano Brivio2022-10-141-0/+4
| | | | | Signed-off-by: Stefano Brivio <sbrivio@redhat.com> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
* log, conf: Add support for logging to fileStefano Brivio2022-10-147-29/+316
| | | | | | | | | | | | | | | | | | | | | | In some environments, such as KubeVirt pods, we might not have a system logger available. We could choose to run in foreground, but this takes away the convenient synchronisation mechanism derived from forking to background when interfaces are ready. Add optional logging to file with -l/--log-file and --log-size. Unfortunately, this means we need to duplicate features that are more appropriately implemented by a system logger, such as rotation. Keep that reasonably simple, by using fallocate() with range collapsing where supported (Linux kernel >= 3.15, extent-based ext4 and XFS) and falling back to an unsophisticated block-by-block moving of entries toward the beginning of the file once we reach the (mandatory) size limit. While at it, clarify the role of LOG_EMERG in passt.c. Signed-off-by: Stefano Brivio <sbrivio@redhat.com> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
* passt.h: Include netinet/if_ether.h before struct ctx declarationStefano Brivio2022-10-141-0/+2
| | | | | | | | This saves some hassle when including passt.h, as we need ETH_ALEN there. Signed-off-by: Stefano Brivio <sbrivio@redhat.com> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
* conf: Drop duplicate, diverging optstring assignmentsStefano Brivio2022-10-141-14/+6
| | | | | | | | | | | | | This originated as a result of copy and paste to introduce a second stage for processing options related to port forwarding, has already bitten David in the past, and just gave me hours of fun. As a matter of fact, the second set of optstring assignments was already incorrect, but it didn't matter because the first one was more restrictive, not allowing optional arguments for -P, -D, -S. Signed-off-by: Stefano Brivio <sbrivio@redhat.com> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
* Move logging functions to a new file, log.cStefano Brivio2022-10-1420-138/+187
| | | | | | | | Logging to file is going to add some further complexity that we don't want to squeeze into util.c. Signed-off-by: Stefano Brivio <sbrivio@redhat.com> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
* test: Add rudimentary support to run selected tests onlyStefano Brivio2022-10-143-2/+53
| | | | | | | | | | | | | | | To keep this simple, only support tests that have corresponding setup and teardown functions implied by their path. For example: ./run passt/ndp will trigger the 'passt' setup and teardown functions. This is not really elegant, but it looks robust, and while David is considering proper alternatives, it should be quite useful. Signed-off-by: Stefano Brivio <sbrivio@redhat.com> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
* Makefile: Hack for optimised-away store in ndp() before checksum calculation2022_09_29.06aa26fStefano Brivio2022-09-292-0/+10
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | With gcc 11 and 12, passing -flto, or -flto=auto, and -O2, intra-procedural optimisation gets rid of a fundamental bit in ndp(): the store of hop_limit in the IPv6 header, before the checksum is calculated, which on x86_64 looks like this: ip6hr->hop_limit = IPPROTO_ICMPV6; b8c0: c6 44 24 35 3a movb $0x3a,0x35(%rsp) Here, hop_limit is temporarily set to the protocol number, to conveniently get the IPv6 pseudo-header for ICMPv6 checksum calculation in memory. With LTO, the assignment just disappears from the binary. This is rather visible as NDP messages get a wrong checksum, namely the expected checksum plus 58, and they're ignored by the guest or in the namespace, meaning we can't get any IPv6 routes, as reported by Wenli Quan. The issue affects a significant number of distribution builds, including the ones for CentOS Stream 9, EPEL 9, Fedora >= 35, Mageia Cauldron, and openSUSE Tumbleweed. As a quick workaround, declare csum_unaligned() as "noipa" for gcc 11 and 12, with -flto and -O2. This disables inlining and cloning, which causes the assignment to be compiled again. Leave a TODO item: we should figure out if a gcc issue has already been reported, and report one otherwise. There's no apparent justification as to why the store could go away. Reported-by: Wenli Quan <wquan@redhat.com> Link: https://bugzilla.redhat.com/show_bug.cgi?id=2129713 Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* udp: Replace pragma to ignore bogus stringop-overread warning with workaroundStefano Brivio2022-09-292-31/+18
| | | | | | | | | | | | | | | | | | | Commit c318ffcb4c93 ("udp: Ignore bogus -Wstringop-overread for write() from gcc 12.1") uses a gcc pragma to ignore a bogus warning, which started appearing on gcc 12.1 (aarch64 and x86_64) due to: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103483 ...but gcc 12.2 has the same issue. Not just that: if LTO is enabled, the pragma itself is ignored (this wasn't the case with gcc 12.1), because of: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80922 Drop the pragma, and assign a frame (in the networking sense) pointer from the offset of the Ethernet header in the buffer, then pass it to write() and pcap(), so that gcc doesn't obsess anymore with the fact that an Ethernet header is 14 bytes and we're sending more than that. Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* Makefile: Extend noinline workarounds for LTO and -O2 to gcc 12Stefano Brivio2022-09-291-3/+3
| | | | | | | | | | | | | | | | Commit 1a563a0cbd49 ("passt: Address gcc 11 warnings") works around an issue where the remote address passed to hash functions is seen as uninitialised by gcc, with -flto and -O2. It turns out we get the same exact behaviour on gcc 12.1 and 12.2, so extend the applicability of the same workaround to gcc 12. Don't go further than that, though: should the issue reported at: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=78993 happen to be fixed in a later version of gcc, we won't need the noinline attributes anymore. Otherwise, we'll notice. Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* cppcheck: Remove unused unmatchedSuppression suppressionsDavid Gibson2022-09-291-3/+0
| | | | | | | | | | | It's unclear what original suppressions these unmatchedSuppression suppressions were supposed to go with. They don't trigger any warnings on the current code that I can tell, so remove them. If we find a problem with some cppcheck versions in future, replace them with inline suppressions so it's clearer exactly where the issue is originating. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* Mark unused functions for cppcheckDavid Gibson2022-09-293-1/+2
| | | | | | | | | | We have a couple of functions that are unused (for now) by design. Although at least one has a flag so that gcc doesn't warn, cppcheck has its own warnings about this. Add specific inline suppressions for these rather than a blanket suppression in the Makefile. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* cppcheck: Remove unused va_list_usedBeforeStarted suppressionDavid Gibson2022-09-291-2/+0
| | | | | | | | | I can't get this warning to trigger, even without the suppression, so remove it. If it shows up again on some cppcheck version, we can replace it with inline suppressions so it's clear where the issue lay. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* cppcheck: Remove unused objectIndex suppressionsDavid Gibson2022-09-291-3/+0
| | | | | | | | | | I can't get these warnings to trigger on the cppcheck versions I have, so remove them. If we find in future we need to replace these, they should be replaced with inline suppressions so its clear what's the section of code at issue. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* cppcheck: Remove unused knownConditionTrueFalse suppressionDavid Gibson2022-09-291-2/+0
| | | | | | | | | | | | | | I can't get this warning to trigger, so I think this suppression must be out of date. Whether that's because we've changed our code to no longer have the problem, or because cppcheck itself has been updated to remove a false positive I don't know. If we find that we do need a suppression like this for some cppcheck version, we should replace it with an inline suppression so it's clear what exactly is triggering the warning. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* cppcheck: Avoid errors due to zeroes in bitwise ORsDavid Gibson2022-09-291-0/+2
| | | | | | | | | | | | Recent versions of cppcheck give warnings if using a bitwise OR (|) where some of the arguments are zero. We're triggering these warnings in our generated seccomp.h header, because BPF_LD and BPF_W are zero-valued. However putting these defines in makes the generate code clearer, even though they could be left out without changing the values. So, add cppcheck suppressions to the generated code. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* Regenerate seccomp.h if seccomp.sh changesDavid Gibson2022-09-291-2/+2
| | | | | | | | seccomp.sh generates seccomp.h, so if we change it, we should re-build seccomp.h as well. Add this to the make dependencies so it happens. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* cppcheck: Suppress NULL pointer warning in tcp_sock_consume()David Gibson2022-09-291-0/+1
| | | | | | | | | | | | | Recent versions of cppcheck give a warning due to the NULL buffer passed to recv() in tcp_sock_consume(). Since this apparently works, I assume it's actually valid, but cppcheck doesn't know that recv() can take a NULL buffer. So, use a suppression to get rid of the error. Also add an unmatchedSuppression suppression since only some cppcheck versions complain about this. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* cppcheck: Suppress same-value-in-ternary branches warningDavid Gibson2022-09-291-0/+1
| | | | | | | | | | | | | | TIMER_INTERVAL is the minimum of two separately defined intervals which happen to have the same value at present. This results in an expression which has the same value in both branches of a ternary operator, which cppcheck warngs about. This is logically sound in this case, so suppress the error (we appear to already have a similar suppression for clang-tidy). Also add an unmatchedSuppression suppression, since only some cppcheck versions complain about this instance. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* qrap: Handle case of PATH environment variable being unsetDavid Gibson2022-09-291-2/+3
| | | | | | | | | | | | | clang-tidy warns that in passing getenv("PATH") to strncpy() we could be passing a NULL pointer. While it's unusual for PATH to be unset, it's not impossible and this would indeed cause getenv() to return NULL. Handle this case by never recognizing argv[2] as a qemu binary name if PATH is not set. This is... no flakier than the detection of whether it's a binary name already is. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* cppcheck: Remove localtime suppression for pcap.cDavid Gibson2022-09-291-1/+0
| | | | | | | | | Since bf95322f "conf: Make the argument to --pcap option mandatory" we no longer call localtime() in pcap.c, so we no longer need the matching cppcheck suppression. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* cppcheck: Broaden suppression for unused struct membersDavid Gibson2022-09-291-2/+1
| | | | | | | | | | | | | | | | In a number of places in passt we use structures to represent over the wire or in-file data with a fixed layout. After initialization we don't access the fields individually and just write the structure as a whole to its destination. Unfortunately cppcheck doesn't cope with this pattern and thinks all the structure members are unused. We already have suppressions for this in pcap.c and dhcp.c However, it also appears in dhcp.c and netlink.c at least. Since this is likely to be common, it seems wiser to just suppress the error globally. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* Avoid ugly 'end' members in netlink structuresDavid Gibson2022-09-291-9/+10
| | | | | | | | | | | | | | We use a number of complex structures to format messages to send to netlink. In some cases we add imaginary 'end' members not because they actually mean something on the wire, but so that we can use offsetof() on the member to determine the relevant size. Adding extra things to the structures for this is kinda nasty. We can use a different construct with offsetof and sizeof to avoid them. As a bonus this removes some cppcheck warnings about unused struct members. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* cppcheck: Use inline suppression for strtok() in conf.cDavid Gibson2022-09-292-1/+2
| | | | | | | | | | | strtok() is non-reentrant and old-fashioned, so cppcheck would complains about its use in conf.c if it weren't suppressed. We're single threaded and strtok() is convenient though, so it's not really worth reworking at this time. Convert this to an inline suppression so it's adjacent to the code its annotating. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* cppcheck: Use inline suppressions for qrap.cDavid Gibson2022-09-292-2/+4
| | | | | | | | | qrap.c uses several old-fashioned functions that cppcheck complains about. Since it's headed for obselesence anyway, just suppress these rather than attempting to modernize the code. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* cppcheck: Use inline suppression for ffsl()David Gibson2022-09-292-2/+1
| | | | | | | | | | We define our own ffsl() as a weak symbol, in case our C library doesn't include it. On glibc systems which *do* include it, this causes a cppcheck warning because unsurprisingly our version doesn't pick the same argument names. Convert the suppression for this into an inline suppression. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* cppcheck: Work around false positive NULL pointer dereference errorDavid Gibson2022-09-291-0/+1
| | | | | | | | | | | | | Some versions of cppcheck could errneously report a NULL pointer deference inside a sizeof(). This is now fixed in cppcheck upstream[0]. For systems using an affected version, add a suppression to work around the bug. Also add an unmatchedSuppression suppression so the suppression itself doesn't cause a warning if you *do* have a fixed cppcheck. [0] https://github.com/danmar/cppcheck/pull/4471 Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* Stricter checking for nsholder.cDavid Gibson2022-09-292-2/+2
| | | | | | | | | | | | Add the -Wextra -pedantic and -std=c99 flags when compiling the nsholder test helper to get extra compiler checks, like we already use for the main source code. While we're there, fix some %d (signed) printf descriptors being used for unsigned values (uid_t and gid_t). Pointed out by cppcheck. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* Don't shadow global function namesDavid Gibson2022-09-291-5/+5
| | | | | | | | cppcheck points out that qrap's main shadows the global err() function with a local. Rename it to rc to avoid the clash. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* Don't shadow 'i' in conf_ports()David Gibson2022-09-291-2/+5
| | | | | | | | | | | | | | The counter 'i' is used in a number of places in conf_ports(), but in one of those we unnecessarily shadow it in an inner scope. We could re-use the same 'i' every time, but each use is logically separate, so instead remove the outer declaration and declare it locally in each of the clauses where we need it. While we're there change it from a signed to unsigned int, since it's used to iterate over port numbers which are generally treated as unsigned. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* cppcheck: Reduce scope of some variablesDavid Gibson2022-09-293-5/+7
| | | | | | | Minor style improvement suggested by cppcheck. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* Clean up parsing in conf_runas()David Gibson2022-09-292-47/+51
| | | | | | | | | | | | | | | | | | | | | | | | | conf_runas() handles several of the different possible cases for the --runas argument in a slightly odd order. Although it can parse both numeric UIDs/GIDs and user/group names, it can't parse a numeric UID combined with a group name or vice versa. That's not obviously useful, but it's slightly surprising gap to have. Rework the parsing to be more systematic: first split the option into user and (optional) group parts, then separately parse each part as either numeric or a name. As a bonus this removes some clang-tidy warnings. While we're there also add cppcheck suppressions for getpwnam() and getgrnam(). It complains about those because they're not reentrant. passt is single threaded though, and is always likely to be during this initialization code, even if we multithread later. There were some existing suppressions for these in the cppcheck invocation but they're no longer up to date. Replace them with inline suppressions which, being next to the code, are more likely to stay correct. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* Pack DHCPv6 "on wire" structuresDavid Gibson2022-09-291-13/+13
| | | | | | | | | | | | | | | | | | | dhcpv6.c contains a number of structures which represent actual DHCPv6 packets as they appear on the wire, which will break if the structures don't have exactly the in-memory layout we expect. Therefore, we should mark these structures as ((packed)). The contents of them means this is unlikely to change the layout in practice - and since it was working, presumably didn't on any arch we were testing on. However it's not impossible for the compiler on some arch to insert unexpected padding in one of these structures, so we should be explicit. clang-tidy warned about this since we were using memcmp() to compare some of these structures, which it thought might not have a unique representation. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
generated by cgit v1.2.3 (git 2.25.1) at 2024-11-01 21:24:42 +0000