From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pf1-x42a.google.com (mail-pf1-x42a.google.com [IPv6:2607:f8b0:4864:20::42a]) by sourceware.org (Postfix) with ESMTPS id 348A93858D32 for ; Tue, 26 Dec 2023 15:39:49 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 348A93858D32 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=linaro.org Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=linaro.org ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 348A93858D32 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=2607:f8b0:4864:20::42a ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1703605191; cv=none; b=fnvylPUrOZolM66lyvRxUKnRNJ8YwdoLZb2niUAZHJzLrzcfIcuUslSo3AmskX8Y9mHfiEDfYtbBA2QUJwQZKM+EeNTKtP5dDSjh0QkN+qi0PRR7IIgoUjvjNnPHHFfYX41/FvxQjFQCK3OK7QNuB2HbuxvyLOXc7ixQkRObsC8= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1703605191; c=relaxed/simple; bh=cDS+ik8wXHyTcFnyaUbP0V42uoOXBf9WRXC274ZU1oA=; h=DKIM-Signature:From:To:Subject:Date:Message-ID:MIME-Version; b=YJ3uvxPtbB+WYJO6LC8JB16qStSLbEIjrRdxlT0wrilITwMSz16Laz9Gwh00VlS99YdoJvPFs5B/5TYUFRmQR1LHGE1fwoJKqBMElFuGPhiVdFB9Hzb20XO9nJoexHjQ6xZkIlpg4DlRR3jK3FItTcyfCRtQnTy/NS+aGY4naCk= ARC-Authentication-Results: i=1; server2.sourceware.org Received: by mail-pf1-x42a.google.com with SMTP id d2e1a72fcca58-6d9b267007fso324508b3a.3 for ; Tue, 26 Dec 2023 07:39:49 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; t=1703605188; x=1704209988; darn=sourceware.org; h=content-transfer-encoding:mime-version:message-id:date:in-reply-to :subject:cc:to:from:user-agent:references:from:to:cc:subject:date :message-id:reply-to; bh=1LWIrTIZsKtUXgZ3/k0nSHWWtBKC8FSGDA3JDVHLgLA=; b=STp4E2BVlWhgtmIFLrgZNDVaExGw6UI116UyQCvoitqcg48FdkbvpM8eMhBtykBlp4 rryYW2W47ANIo/6M23JrA6d8uRX2bwRcAnD6oihNGnl96/fS+R5OCRY2s9qaVUT07lcA /kFzZO4bnbEkzGEEcGuEnSmGwqjZYtMDgCqkANd6hYJakVdA3TwmDQvDjIY0FO/IBFl1 lwUBz8TQ3pTBOT6sNUc2xUKSmWUy9u8g71Kn+Ta62Q6XX/8GNZV04gnVY/eqe3aZhKYb IcHT53R3TKICl9ArTYylEEm7K0Vc/MLiP6ZYoNe6nPLt6T7gbxDq8FeAh7ntSGPuQS0a QqMQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1703605188; x=1704209988; h=content-transfer-encoding:mime-version:message-id:date:in-reply-to :subject:cc:to:from:user-agent:references:x-gm-message-state:from:to :cc:subject:date:message-id:reply-to; bh=1LWIrTIZsKtUXgZ3/k0nSHWWtBKC8FSGDA3JDVHLgLA=; b=bYnqeZwodu8ESDrPdEeQkm/cGrkUhBsfFibDeQAHJsUFha021m9kYxHNjLp8ZOcPO1 4Dy7A8hatV8t1zA0+rHG6mh8rJCR1NCSpDZrPj9/CfwKjfVAXP2UW/ninoWfhBaPddCl znCpyWsK8gdteW2TE/p5cpiaDaH1f2P/GptZjSZ7+q9XYA4BgimiW+15D/RbE5wVKH2Q jJNzVJNfgN3kTSF73CWRkdEuDN0ypRk/BBy0vTgiJilAPwaUs6ntqh32iQiaR+233Hu5 hmkLJN5LLWTJU3jTcvF9YMG/QZscW0di/Mh1wiE55Rp+EV87gmB2uT51OJFzoBPQih5h 82ww== X-Gm-Message-State: AOJu0Yw86ZgBBm7fnBfzz6O9FklCFEfOI5/33ZX7ZLNZF6YUJoLhnen0 O3GAtUzrw1fJ7JOV4i7m8R3I5WojQh3/FUUAgtg+k2G7sz8= X-Google-Smtp-Source: AGHT+IFCIoQaV9E+wvJfV4M0TJda0Yb0mFSwaaEfLzg8c/iIfrtEKP3fJ7vDkoM7AwdflIMpRslc4Q== X-Received: by 2002:aa7:8b5a:0:b0:6d7:d890:2838 with SMTP id i26-20020aa78b5a000000b006d7d8902838mr2683412pfd.67.1703605188079; Tue, 26 Dec 2023 07:39:48 -0800 (PST) Received: from localhost ([2804:14d:7e39:8470:b425:ec2d:a09a:544f]) by smtp.gmail.com with ESMTPSA id i19-20020aa787d3000000b006d9f1be0fd1sm663964pfo.214.2023.12.26.07.39.47 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 26 Dec 2023 07:39:47 -0800 (PST) References: <20231222113359.1224157-1-ahajkova@redhat.com> User-agent: mu4e 1.10.8; emacs 29.1 From: Thiago Jung Bauermann To: Alexandra =?utf-8?B?SMOhamtvdsOh?= Cc: gdb-patches@sourceware.org Subject: Re: [PATCH] remote.c: Make packet_check_result return a structure In-reply-to: <20231222113359.1224157-1-ahajkova@redhat.com> Date: Tue, 26 Dec 2023 12:39:45 -0300 Message-ID: <87wmt1ezwe.fsf@linaro.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-Spam-Status: No, score=-4.2 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_NONE,SPF_HELO_NONE,SPF_PASS,TXREP,T_SCC_BODY_TEXT_LINE 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: Hello, Nice change! Just a couple of small nits below, if you agree with them. Regardless of whether you make the changes I suggest: Reviewed-by: Thiago Jung Bauermann Alexandra H=C3=A1jkov=C3=A1 writes: > @@ -9705,8 +9751,9 @@ remote_target::remote_send_printf (const char *form= at, ...) >=20=20 > rs->buf[0] =3D '\0'; > getpkt (&rs->buf); > + packet_result pkt_status =3D packet_check_result (rs->buf); IMHO it's a bit confusing to have a variable of type packet_result named pkt_status. I'd suggest changing to pkt_result. >=20=20 > - return packet_check_result (rs->buf); > + return pkt_status.status (); > } >=20=20 > /* Flash writing can take quite some time. We'll set > @@ -9718,7 +9765,7 @@ void > remote_target::flash_erase (ULONGEST address, LONGEST length) > { > int addr_size =3D gdbarch_addr_bit (current_inferior ()->arch ()) / 8; > - enum packet_result ret; > + enum packet_status ret; > scoped_restore restore_timeout > =3D make_scoped_restore (&remote_timeout, remote_flash_timeout); >=20=20 > @@ -11308,7 +11355,7 @@ remote_target::verify_memory (const gdb_byte *dat= a, CORE_ADDR lma, ULONGEST size > if (target_has_execution () > && m_features.packet_support (PACKET_qCRC) !=3D PACKET_DISABLE) > { > - enum packet_result result; > + enum packet_status result; Similar comment here. A variable of type packet_status is named result. The upside of preserving the current name is that the diff is smaller, but I think the clearer code is worth having a bigger diff. Same comment for the following hunks of the patch: >=20=20 > /* Make sure the remote is pointing at the right process. */ > set_general_process (); > @@ -12210,7 +12257,7 @@ remote_target::get_thread_local_address (ptid_t p= tid, CORE_ADDR lm, > struct remote_state *rs =3D get_remote_state (); > char *p =3D rs->buf.data (); > char *endp =3D p + get_remote_packet_size (); > - enum packet_result result; > + enum packet_status result; >=20=20 > strcpy (p, "qGetTLSAddr:"); > p +=3D strlen (p); > @@ -12256,7 +12303,7 @@ remote_target::get_tib_address (ptid_t ptid, CORE= _ADDR *addr) > struct remote_state *rs =3D get_remote_state (); > char *p =3D rs->buf.data (); > char *endp =3D p + get_remote_packet_size (); > - enum packet_result result; > + enum packet_status result; >=20=20 > strcpy (p, "qGetTIBAddr:"); > p +=3D strlen (p); > @@ -13821,7 +13868,7 @@ remote_target::get_trace_status (struct trace_sta= tus *ts) > { > /* Initialize it just to avoid a GCC false warning. */ > char *p =3D NULL; > - enum packet_result result; > + enum packet_status result; > struct remote_state *rs =3D get_remote_state (); >=20=20 > if (m_features.packet_support (PACKET_qTStatus) =3D=3D PACKET_DISABLE) > @@ -14201,7 +14248,7 @@ remote_target::set_trace_buffer_size (LONGEST val) > struct remote_state *rs =3D get_remote_state (); > char *buf =3D rs->buf.data (); > char *endbuf =3D buf + get_remote_packet_size (); > - enum packet_result result; > + enum packet_status result; >=20=20 > gdb_assert (val >=3D 0 || val =3D=3D -1); > buf +=3D xsnprintf (buf, endbuf - buf, "QTBuffer:size:"); > @@ -15527,9 +15574,10 @@ remote_target::store_memtags (CORE_ADDR address,= size_t len, >=20=20 > putpkt (rs->buf); > getpkt (&rs->buf); > + packet_result pkt_status =3D packet_check_result (rs->buf); >=20=20 > /* Verify if the request was successful. */ > - return packet_check_result (rs->buf.data ()) =3D=3D PACKET_OK; > + return pkt_status.status () =3D=3D PACKET_OK; > } >=20=20 > /* Return true if remote target T is non-stop. */ --=20 Thiago