From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca (simark.ca [158.69.221.121]) by sourceware.org (Postfix) with ESMTPS id 912303857809 for ; Mon, 15 Nov 2021 19:29:45 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 912303857809 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=simark.ca Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=simark.ca Received: from [172.16.0.95] (192-222-180-24.qc.cable.ebox.net [192.222.180.24]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id 0FEFB1E940; Mon, 15 Nov 2021 14:29:45 -0500 (EST) Subject: Re: [PATCHv6 3/3] gdb/python: add gdb.RemoteTargetConnection.send_packet To: Andrew Burgess , gdb-patches@sourceware.org Cc: Andrew Burgess References: <1ee3ff1661f4e29adb8cf1daf90f88bd7c282a77.1636997240.git.aburgess@redhat.com> From: Simon Marchi Message-ID: Date: Mon, 15 Nov 2021 14:29:44 -0500 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.13.0 MIME-Version: 1.0 In-Reply-To: <1ee3ff1661f4e29adb8cf1daf90f88bd7c282a77.1636997240.git.aburgess@redhat.com> Content-Type: text/plain; charset=utf-8 Content-Language: tl Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-5.3 required=5.0 tests=BAYES_00, KAM_DMARC_STATUS, NICE_REPLY_A, SPF_HELO_PASS, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 15 Nov 2021 19:29:47 -0000 > @@ -6062,6 +6083,42 @@ > to the remote target. > @end defvar > > +The @code{gdb.RemoteTargetConnection} class is a sub-class of > +@code{gdb.TargetConnection}, and is used to represent @samp{remote} > +and @samp{extended-remote} connections. In addition to the attributes > +and methods available from the @code{gdb.TargetConnection} base class, > +a @code{gdb.RemoteTargetConnection} has the following method: > + > +@kindex maint packet > +@defun RemoteTargetConnection.send_packet (@var{packet}) > +This method sends @var{packet}, which should be a non-empty string or > +bytes array, to the remote target and returns the response. If > +@var{packet} is not a string, a bytes array, or is empty, then an > +exception of type @code{ValueError} is thrown. If the type is wrong, a TypeError error is raised in fact. Minor nit: Python uses "raise" and not "throw", for exceptions. I suppose our doc should follow that nomenclature when talking about Python code. > + > +/* Implement RemoteTargetConnection.send_packet function. Send a packet to > + the target identified by SELF. The connection must still be valid, and > + the packet to be sent must be non-empty, otherwise an exception will be > + thrown. */ > + > +static PyObject * > +connpy_send_packet (PyObject *self, PyObject *args, PyObject *kw) > +{ > + connection_object *conn = (connection_object *) self; > + > + CONNPY_REQUIRE_VALID (conn); > + > + static const char *keywords[] = {"packet", nullptr}; > + const char *packet_str = nullptr; > + Py_ssize_t packet_len; > + > + if (!gdb_PyArg_ParseTupleAndKeywords (args, kw, "s#", keywords, > + &packet_str, &packet_len)) > + return nullptr; I tried my example yet again: res = conn.send_packet('X555555558010,4:\xff\x03\x02\x01') ... and that still produced the error I got with the previous patch series version. This is expected, as the unicode string still gets encoded as UTF-8, and that generates 5 bytes of data instead of 4. What I have to do is pass in a bytes object, not a string: res = conn.send_packet(b'X555555558010,4:\xff\x03\x02\x01') I think we should only accept bytes-like objects here (y#). Accepting string objects and implicitly encoding them to bytes using the utf-8 codec (as the `s#` specifier does) just sets up a trap for people to fall into, like I did. If people really want to send the bytes representing the UTF-8 encoding of some string, they can just encode the bytes themselves, from a string: In [1]: "é".encode() Out[1]: b'\xc3\xa9' This way, there's no misunderstanding about what the user wants. I think that for Python 2 we could use `s#`, as `y#` doesn't exist (again, str in Python 3 is basically Python 3's bytes). The doc [1] says about s#: Convert a Python string or Unicode object to a C pointer to a character string. This variant on s stores into two C variables, the first one a pointer to a character string, the second one its length. In this case the Python string may contain embedded null bytes. Unicode objects pass back a pointer to the default encoded string version of the object if such a conversion is possible. So with Python 2, people could still pass a unicode object and fall into the same trap I fell in. But the difference is that with Python 2, if you want to use a unicode object, you have to do so explicitly. So it won't happen by mistake. So the only difference in the GDB code for Python 2 and 3 would be the format specifier. [1] https://docs.python.org/2.7/c-api/arg.html > + if (packet_str == nullptr || *packet_str == '\0' || packet_len <= 0) I'm pretty sure that packet_str can't be nullptr here, if there's a problem with the type of the argument, gdb_PyArg_ParseTupleAndKeywords will raise and we will return above. I would use a gdb_assert for this instead. I would probably drop the *packet_str == '\0' test. While it's not very useful to send a packet with a '\0' in first position, I don't see why we should discriminate against it specifically. The caller sends what it wants to send. > + { > + PyErr_SetString (PyExc_ValueError, _("Invalid remote packet")); > + return nullptr; ... and that would only leave "packet_len <= 0", which would happen if the passed-in data is empty. I would make the message a bit more precise then: PyErr_SetString (PyExc_ValueError, _("Packet must not be empty")); > + } > + > + try > + { > + scoped_restore_current_thread restore_thread; > + switch_to_target_no_thread (conn->target); > + > + gdb::array_view view (packet_str, packet_len); > + py_send_packet_callbacks callbacks; > + send_remote_packet (view, &callbacks); > + return callbacks.result ().release (); Assert that callbacks.result != nullptr? Simon