public inbox for
 help / color / mirror / Atom feed
From: Andrew Burgess <>
To: Alexandra Petlanova Hajkova <>
Subject: Re: [PATCH v2] remote.c: Allow inferior to reply with an error
Date: Wed, 18 Jan 2023 18:19:40 +0000	[thread overview]
Message-ID: <> (raw)
In-Reply-To: <>

Alexandra Petlanova Hajkova <> writes:

>> So, I started reviewing this patch, and wrote a whole bunch of stuff.
>> One thing I wrote was, you should really add a test for this that uses
>> the existing gdbserver.
>  I haven't added one because I wasn't sure this patch would be accepted.
> I'll add one if we agree this work makes sense.
>> So, I thought, I wonder what errors gdbserver currently sends back, so I
>> took a look at the QSetWorkingDir handling in gdbserver/
>> Weird, I thought, it doesn't appear to send back any errors.
>> So then I took a look at set_inferior_cwd in gdbserver/, and
>> all that it does is cache the requested directory.
>> So, then I went to the docs again, and re-read what we say about
>> QSetWorkingDir:
>>   This packet is used to inform the remote server of the intended
>>   current working directory for programs that are going to be executed.
>> Note the use of the word 'intended'.  This packet does not try to change
>> the directory NOW, it will try to change the directory LATER, when the
>> inferior is actually executed.
>> My current reading of the docs is that the QSetWorkingDir packet, is
>> either not supported, or should never fail.
>> If you specify an invalid directory then future attempts to start an
>> inferior will fail, but the QSetWorkingDir packet itself shouldn't
>> fail.
> I think I misinterpreted this part. My understanding was the remote
> gdbserver (or the stub) will change the directory and then it's weird
> to not be able to send back errors. Your interpretation makes perfect
> sense.
> In this case, if this behaviour is the preferred way to handle the
> QSetWOrkingDir,
> I propose to change the documentation to make it better understandable.
>> I guess we could decide that we want to extend QSetWorkingDir to allow
>> for it to send back error packets, I'm not sure that's entirely a good
>> idea though as that's not how the existing gdbserver works... but if we
>> did want to do that then we should probably improve the documentation to
>> explain that some gdbservers will cache the passed in directory and
>> return an error later, while others might choose to immediately change
>> directory and return an error now.
>> Because I already wrote it, I've left all my original review comments
>> inline below, but I'm not sure if you will want to roll a v3 or not.
>> What is your preference and does anybody else have an opinion on
> QSetWorkingDir
> (and similar packets) behaviour?
> Another confusing thing in remote protocol documentation (
> is the error handling. Various packets can reply with an error:
> ‘E NN’

Indeed, and laying this out at 'E NN' seems really bad to me as there's
no space in the packet.  Though looking through the docs we seem to do
this everywhere, so I guess anyone working on this will figure that bit
our pretty quick.

> Indicate a badly formed request. The error number NN is given as hex
> digits.
> But packet_check_result in remote.c just checks if it's an error and does
> not handle the 'NN'. And I haven't found the meaning of 'NN'.

Just throwing out an idea here, maybe packet_check_result should return
an object, not an enum.  Maybe a packet_result.  Properties of this
object would be:

  struct packet_result
     /* Return the type of packet we encountered.  */
     enum packet_result status () const;

     /* Return a string representing the error, only valid if status()
        is PACKET_ERROR.  */
     std::string error_message () const;

Then packet_check_result could parse the packet just like it currently
does.  If it sees 'Enn' then the error message becomes 'Unknown error:
nn', if it sees 'E.msg' then the error message becomes the 'msg' part of
the packet.

We could even imagine a future where callers of packet_check_result
would pass in a error code translation table that maps values of 'nn' to
defined error messages ... maybe?

I feel this is drifting away from your original patch, but is an
interesting idea of better error reporting.


> Thank you very much for the in-depth review.

      reply	other threads:[~2023-01-18 18:19 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-01-13 11:59 
2023-01-13 13:16 ` Eli Zaretskii
2023-01-17 16:30 ` Andrew Burgess
2023-01-18  9:48   ` Mark Wielaard
2023-01-18 18:10     ` Andrew Burgess
2023-01-18 13:37   ` Alexandra Petlanova Hajkova
2023-01-18 18:19     ` Andrew Burgess [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \ \ \ \

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).