Daniel P. Berrangé <berra...@redhat.com> writes:
> On Tue, Jul 15, 2025 at 10:35:16AM +0200, Philippe Mathieu-Daudé wrote:
>> @errp is always NULL. Remove it, as unused.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <phi...@linaro.org>
>> ---
>> include/system/os-win32.h | 2 +-
>> io/channel-socket.c | 4 ++--
>> util/oslib-win32.c | 6 +++---
>> 3 files changed, 6 insertions(+), 6 deletions(-)
>>
>> diff --git a/include/system/os-win32.h b/include/system/os-win32.h
>> index 3aa6cee4c23..40712a948c3 100644
>> --- a/include/system/os-win32.h
>> +++ b/include/system/os-win32.h
>> @@ -172,7 +172,7 @@ static inline void qemu_funlockfile(FILE *f)
>> bool qemu_socket_select(int sockfd, WSAEVENT hEventObject,
>> long lNetworkEvents, Error **errp);
>>
>> -bool qemu_socket_unselect(int sockfd, Error **errp);
>> +bool qemu_socket_unselect(int sockfd);
>>
>> /* We wrap all the sockets functions so that we can set errno based on
>> * WSAGetLastError(), and use file-descriptors instead of SOCKET.
>> diff --git a/io/channel-socket.c b/io/channel-socket.c
>> index 3b7ca924ff3..6ee6217e7ac 100644
>> --- a/io/channel-socket.c
>> +++ b/io/channel-socket.c
>> @@ -454,7 +454,7 @@ static void qio_channel_socket_finalize(Object *obj)
static void qio_channel_socket_finalize(Object *obj)
{
QIOChannelSocket *ioc = QIO_CHANNEL_SOCKET(obj);
if (ioc->fd != -1) {
QIOChannel *ioc_local = QIO_CHANNEL(ioc);
if (qio_channel_has_feature(ioc_local, QIO_CHANNEL_FEATURE_LISTEN))
{
Error *err = NULL;
socket_listen_cleanup(ioc->fd, &err);
if (err) {
error_report_err(err);
err = NULL;
>> }
>> }
>> #ifdef WIN32
>> - qemu_socket_unselect(ioc->fd, NULL);
>> + qemu_socket_unselect(ioc->fd);
>> #endif
>
> It seems to me like this code should instead be using
> &error_warn, because the errors are still relevant and
> potentially a sign of a bug, but we don't want to stop
> this finalization path.
Would such a warning be actionable for the user?
Why is this failure a warning, but the failure right above is an error?
What are the possible failures?
On reporting errors with error_report() & friends: doing so within a
function that uses an Error **errp parameter to return errors is almost
always wrong. Can qio_channel_socket_finalize() run within such a
function?
>
>> close(ioc->fd);
>> ioc->fd = -1;
>> @@ -890,7 +890,7 @@ qio_channel_socket_close(QIOChannel *ioc,
>>
>> if (sioc->fd != -1) {
>> #ifdef WIN32
>> - qemu_socket_unselect(sioc->fd, NULL);
>> + qemu_socket_unselect(sioc->fd);
>> #endif
>
> Here too, we don't want to stop the close operation early,
> as we need to 'close()' the FD, but we should diagnose
> the failure none the less
Same questions.
>> if (qio_channel_has_feature(ioc, QIO_CHANNEL_FEATURE_LISTEN)) {
>> socket_listen_cleanup(sioc->fd, errp);
[...]
