Re: [PATCH v2 10/37] system: propagate Error to gdbserver_start (and other device setups)

[Alex Bennée]

Ilya Leoshkevich <iii@linux.ibm.com> writes:

> On Tue, 2025-01-14 at 11:37 +0000, Alex Bennée wrote:
>> This started as a clean-up to properly pass a Error handler to the
>> gdbserver_start so we could do the right thing for command line and
>> HMP invocations.
>> 
>> Now that we have cleaned up foreach_device_config_or_exit() in
>> earlier
>> patches we can further simplify by it by passing &error_fatal instead
>> of checking the return value. Having a return value is still useful
>> for HMP though so tweak the return to use a simple bool instead.
>> 
>> Message-Id: <20250109170619.2271193-11-alex.bennee@linaro.org>
>> Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> Acked-by: Ilya Leoshkevich <iii@linux.ibm.com>
>
> Apparently the BSD code needs to be adjusted:
>
> ../qemu/bsd-user/main.c:631:32: error: too few arguments to function
> call, expected 2, have 1
>   631 |         gdbserver_start(gdbstub);
>       |         ~~~~~~~~~~~~~~~        ^
> /home/user/qemu/include/exec/gdbstub.h:63:6: note: 'gdbserver_start'
> declared here
>    63 | bool gdbserver_start(const char *port_or_device, Error **errp);
>       |      ^               ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 1 error generated.
>
>> ---
>> v2
>>   - split some work into pre-cursor patches
>> ---
>>  include/exec/gdbstub.h |  8 +++++-
>>  gdbstub/system.c       | 22 ++++++++--------
>>  gdbstub/user.c         | 20 ++++++++-------
>>  linux-user/main.c      |  6 +----
>>  monitor/hmp-cmds.c     |  2 +-
>>  system/vl.c            | 58 ++++++++++++++++++++--------------------
>> --
>>  6 files changed, 59 insertions(+), 57 deletions(-)
>
> [...]
>
>> diff --git a/gdbstub/user.c b/gdbstub/user.c
>> index 0b4bfa9c48..fb8f6867ea 100644
>> --- a/gdbstub/user.c
>> +++ b/gdbstub/user.c
>> @@ -13,6 +13,7 @@
>>  #include "qemu/bitops.h"
>>  #include "qemu/cutils.h"
>>  #include "qemu/sockets.h"
>> +#include "qapi/error.h"
>>  #include "exec/hwaddr.h"
>>  #include "exec/tb-flush.h"
>>  #include "exec/gdbstub.h"
>> @@ -372,15 +373,15 @@ static bool gdb_accept_tcp(int gdb_fd)
>>      return true;
>>  }
>>  
>> -static int gdbserver_open_port(int port)
>> +static int gdbserver_open_port(int port, Error **errp)
>>  {
>>      struct sockaddr_in sockaddr;
>>      int fd, ret;
>>  
>>      fd = socket(PF_INET, SOCK_STREAM, 0);
>>      if (fd < 0) {
>> -        perror("socket");
>> -        return -1;
>> +        error_setg(errp, "Failed to bind socket: %s",
>> strerror(errno));
>> +        return false;
>
> I think return -1 needs to stay here.
>
>>      }
>>      qemu_set_cloexec(fd);
>
> [...]

Will fix. Thanks.

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro