GSocketConnection/GOutputStream/GInputStream seems rather unnecessary.
Maybe they make sense when you want to write portable code (for
Windows). Otherwise, watching a file descriptor and reading/writing it
directly is simpler (and also more efficient).
For example, we passed no GCancellable to g_input_stream_read_async().
What does that mean w.r.t. destroying the NMOvsdb instance? I suspect
it's wrong, but it's hard to say, because there are so many layers of
code.
Note that we anyway keep state in NMOvsdb, namely the data we want to
send (output_buf) and the data we partially received (input_buf). All we
need, are poll notifications when the file descriptor is ready. To
those, we hook up the read/write callbacks. Also before was the code
async, and there were callbacks when when read/write was done. That does
not simplify the code in any way.
- we no longer use separate NMOvsdbPrivate.buf and NMOvsdbPrivate.input
buffers. There is just a NMOvsdbPrivate.input_buf that can we can fill
directly.
The "priv->bufp" offset is only used while parsing a message at a time.
It's unnecessary to track it in NMOvsdbPrivate and keep it between
parsing messages. Tracking the state in NMOvsdbPrivate makes it more
complicated to understand, because one needs to reason at which times
the state is used (when it really is not used).
Also, move the parsing to a separate function.
We did not initialize "child_stderr". If that were necessary, we would need
to add it too. However, it is clearly not necessary to initialize those fields.
It's not used. It's better to use SOCK_NONBLOCK flag for socket(), as we do.
Also, the implementation that blindly calls F_SETFL without merging the
existing flags from F_GETFL is just wrong. Drop it altogether.
F_SETFL will reset the flags. That is wrong, as we only want to add
O_NONBLOCK flag and leaving the other flags alone. Usually, we would
need to call F_GETFL first.
Note that on Linux, F_SETFL can only set certain flags, so the
O_RDWR|O_CLOEXEC flags were unaffected by this. That means, most likely
there are no other flags that our use of F_SETFL would wrongly clear.
Still, it's ugly, because it's not obvious whether there might be other
flags.
Avoid that altogether, by setting the flag already during open().
Fixes: 67e092abcb ('core: better handling of rfkill for WiMAX and WiFi (bgo #629589) (rh #599002)')
The device shouldn't change state from DEACTIVATING to DISCONNECTED
until its detached from its controller; otherwise, the port detach
that is in progress can conflict with the following activation.
This changes the signature of detach_port() to be asynchronous,
similarly to attach_port(). The implementation can return TRUE/FALSE
on immediate completion.
Current implementations return immediately and so there is no change
in behavior for now.
It is wrong trying to send the signal still. Just error out.
Note that ECHILD indicates that the process is either not a child
or was already reaped. In both cases, that is a bug of the caller
who must keep accurate track of the child's process ID.
It's easy enough to know how many bytes are needed. Just allocate the
right size (+1, because NMStrBuf really likes to reserve that extra byte
for the trailing NUL, even if it's not needed in this case).
src/libnm-core-impl/tests/test-keyfile.c: In function '_invalid_option_write_handler':
src/libnm-core-impl/tests/test-keyfile.c:917:9: error: 'message' may be used uninitialized [-Werror=maybe-uninitialized]
917 | g_assert(message && strstr(message, "ethtool.bogus"));
| ^
src/libnm-core-impl/tests/test-keyfile.c:905:29: note: 'message' was declared here
905 | const char *message;
| ^
lto1: all warnings being treated as errors
"preexec_fn" is not great, because it is not generally safe in multi
threaded code (and we don't know whether the test didn't start other
threads already, like a GDBus worker thread). Well, it probably is safe,
if you only call async signal safe code, but it's not clear what os.dup2()
does under the hood.
Just avoid that. We can pass on the FD directly.
test_ec2 (__main__.TestNmCloudSetup.test_ec2) ... /usr/lib64/python3.11/unittest/case.py:579: ResourceWarning: unclosed file <_io.BufferedWriter name=5>
if method() is not None:
ResourceWarning: Enable tracemalloc to get the object allocation traceback
ok
Fixes: d89d42bf23 ('tests/client: test nm-cloud-setup')
This seems unnecessary, because we spawn the child process via subprocess.Popen and
set "pass_fds". That already ensures to pass on the FD.
Worse, socket.set_inheritable() is only added in Python 3.4, that means the
test is gonna break for Python 3.2 and 3.3. Just avoid that by not using the
unnecessary function. For the same reason, drop "inheritable=True" from
os.dup2(). "inheritable=True" is already the default, but only exists
since Python 3.4.
Fixes: d89d42bf23 ('tests/client: test nm-cloud-setup')
The test uses subprocess.Popen()'s "pass_fd" argument. That is only
available since Python 3.2. Possibly it could be solved differently, but
that is not implemented. Instead, skip the test.
Also, socket.socket.set_inheritable() is Python 3.4. But presumably
we don't need it.
Fixes: d89d42bf23 ('tests/client: test nm-cloud-setup')
We have other places like
nm_assert(!out_seq_result || *out_seq_result == WAIT_FOR_NL_RESPONSE_RESULT_UNKNOWN);
where we explicitly compare against WAIT_FOR_NL_RESPONSE_RESULT_UNKNOWN.
Do that here too.
With a small buffer (of 4K) and many links (100 ethernet adapters), I've
seen up to ~15 retries of link change until things settled.
Let's increase this. Still a »bulharská konštanta« but possibly safer and
more broadly useful (so we can cap the link change retry count too).
