On 15.09.25 20:04, Jag Raman wrote:
On Sep 15, 2025, at 9:22 AM, Vladimir Sementsov-Ogievskiy
<[email protected]> wrote:
Currently, we just always pass NULL as errp argument. That doesn't
look good.
Some realizations of interface may actually report errors.
Channel-socket realization actually either ignore or crash on
errors, but we are going to straighten it out to always reporting
an errp in further commits.
So, convert all callers to either handle the error (where environment
allows) or explicitly use &error_abort.
Take also a chance to change the return value to more convenient
bool (keeping also in mind, that underlying realizations may
return -1 on failure, not -errno).
Suggested-by: Daniel P. Berrangé <[email protected]>
Signed-off-by: Vladimir Sementsov-Ogievskiy <[email protected]>
Thanks for the patch, Vladimir! LGTM, please see some nits below
Reviewed-by: Jagannathan Raman <[email protected]>
Thanks for reviewing!
---
block/nbd.c | 5 ++++-
chardev/char-socket.c | 14 ++++++++++----
hw/remote/proxy.c | 6 +++++-
[..]
diff --git a/nbd/server.c b/nbd/server.c
index d242be9811..acec0487a8 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -1411,7 +1411,9 @@ static coroutine_fn int nbd_negotiate(NBDClient *client,
Error **errp)
....options sent, ending in NBD_OPT_EXPORT_NAME or NBD_OPT_GO....
*/
- qio_channel_set_blocking(client->ioc, false, NULL);
+ if (!qio_channel_set_blocking(client->ioc, false, errp)) {
+ return -EINVAL;
Should we consider a different errno here? Perhaps EIO?
I still think, that EINVAL is better, as this opertions is not about IO, it's
just changing flags of file descriptor. The actual errno is lost at this stage,
but looking at fcntl (which should be the underlying mechanism) spec, it could
be one of EACCES, EAGAIN, EBADF, EINVAL. And EINVAL is good default for "just
an error".
+ }
qio_channel_set_follow_coroutine_ctx(client->ioc, true);
trace_nbd_negotiate_begin();
[..]
diff --git a/tests/unit/io-channel-helpers.c b/tests/unit/io-channel-helpers.c
index c0799c21c2..22b42d14cd 100644
--- a/tests/unit/io-channel-helpers.c
+++ b/tests/unit/io-channel-helpers.c
@@ -20,6 +20,7 @@
#include "qemu/osdep.h"
#include "io-channel-helpers.h"
+#include "qapi/error.h"
#include "qemu/iov.h"
struct QIOChannelTest {
@@ -109,8 +110,8 @@ void qio_channel_test_run_threads(QIOChannelTest *test,
test->src = src;
test->dst = dst;
- qio_channel_set_blocking(test->dst, blocking, NULL);
- qio_channel_set_blocking(test->src, blocking, NULL);
+ qio_channel_set_blocking(test->dst, blocking, &error_abort);
+ qio_channel_set_blocking(test->src, blocking, &error_abort);
Do we need to assert that the call to qio_channel_set_blocking()
was successful, similar to what qio_channel_test_validate() does
with other function calls?
No, &error_abort does this work for us: in case of error it will
print the error message and abort (look at error_handle() in
util/error.c)
--
Best regards,
Vladimir
