From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by sourceware.org (Postfix) with ESMTPS id E51823858430 for ; Wed, 18 Jan 2023 13:37:42 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org E51823858430 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1674049062; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=P5bts44GmxlHMYyaANTsy/8cmhVP/+4VbhwRzuSLGMA=; b=H7vXT/TbJ2qw46lmfdRayM0wgZS7xoIb79DaK2srOgs2plXdYcAyTyyLREZoZpeD8T2GX0 pw83typVBK85+suge51hM+7ODd0bfnsdYcPvNTDN3ossjc6j0gSadmkwA4uXYLTfoUt7uL OJbGVZR70DV+yNSArYoMA6wBNjcWFAE= Received: from mail-pj1-f71.google.com (mail-pj1-f71.google.com [209.85.216.71]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_128_GCM_SHA256) id us-mta-669-Y3hiZXORMHyA9i649KozwA-1; Wed, 18 Jan 2023 08:37:41 -0500 X-MC-Unique: Y3hiZXORMHyA9i649KozwA-1 Received: by mail-pj1-f71.google.com with SMTP id q9-20020a17090a304900b00226e84c4880so14389627pjl.4 for ; Wed, 18 Jan 2023 05:37:40 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=P5bts44GmxlHMYyaANTsy/8cmhVP/+4VbhwRzuSLGMA=; b=bH7O74Y08AwZe8t91bQlVPCBpahlhl4gM0E7xDaXgsffcZzqhDjtjvb6hvBLLNb0E/ axREP+EoagRQgIkEKEDfWzyKoGWHAn30lXj7ZKmcrLauV+HBet7/ph8S2Bg+rrkRCDLY hDFFIKaI3aAQT+DFUqXL9gAsl4abxzX3mTsSi5RM23KxdzJAWXEjOUp169CuPC0zCqC6 ByH+pGo+gbts/lB30K0UwQ4FdOEj7RlR/odnw3mt5JCTZ1T1Yr58SdiePOCb6kXDQnXn 1kUAZEtZF5v39pavwa8QSuVM0PV/+/wI5FHOW56bl3gdZU3WTorqUBeW/0DRR2jxDJpv rxyA== X-Gm-Message-State: AFqh2krC9aMcOfVZoPOCw2fUcjz5npEsxeXrWy5Ia4G1vOJqwkCRALzy HfZkclzwWo+i1/eJxWwcQodsdqq+OTUIjmILl97hxy7I09+fjxMOjyIf8fjrVGtpiYrBJzGoDiQ T8bLSvjSyj70nlppvoJvhJ5GUAXxOD9Jpf0F3JA== X-Received: by 2002:a63:6402:0:b0:478:f77e:4cb0 with SMTP id y2-20020a636402000000b00478f77e4cb0mr468920pgb.104.1674049059882; Wed, 18 Jan 2023 05:37:39 -0800 (PST) X-Google-Smtp-Source: AMrXdXsC/Ro1Jbq4OmBgfgBqSLdsyaaoyiE6GuNC0WUK1fr2eiu1Z254F1Dzh58jhMzY2DsXPzWpUWby3oxrNO1uLUs= X-Received: by 2002:a63:6402:0:b0:478:f77e:4cb0 with SMTP id y2-20020a636402000000b00478f77e4cb0mr468915pgb.104.1674049059541; Wed, 18 Jan 2023 05:37:39 -0800 (PST) MIME-Version: 1.0 References: <20230113115910.3215524-1-ahajkova@redhat.com> <87zgahdsyi.fsf@redhat.com> In-Reply-To: <87zgahdsyi.fsf@redhat.com> From: Alexandra Petlanova Hajkova Date: Wed, 18 Jan 2023 14:37:28 +0100 Message-ID: Subject: Re: [PATCH v2] remote.c: Allow inferior to reply with an error To: Andrew Burgess Cc: AlexandraH=?UTF-8?B?w6E=?=jkov=?UTF-8@sourceware.org, ?B?w6E=?=@sourceware.org, gdb-patches@sourceware.org X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: multipart/alternative; boundary="00000000000087f31d05f289eb07" X-Spam-Status: No, score=-3.8 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,HTML_MESSAGE,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,SPF_HELO_NONE,SPF_NONE,TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: --00000000000087f31d05f289eb07 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 8bit > > 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/server.cc. > Weird, I thought, it doesn't appear to send back any errors. > > So then I took a look at set_inferior_cwd in gdbserver/inferiors.cc, 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 ( https://sourceware.org/gdb/onlinedocs/gdb/General-Query-Packets.html#General-Query-Packets) is the error handling. Various packets can reply with an error: ‘E NN’ 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'. Thank you very much for the in-depth review. --00000000000087f31d05f289eb07--