git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Josh Steadmon <steadmon@google.com>
To: Jeff Hostetler <git@jeffhostetler.com>
Cc: git@vger.kernel.org, Jeff Hostetler <jeffhost@microsoft.com>
Subject: Re: [PATCH 00/10] Advertise trace2 SID in protocol capabilities
Date: Mon, 2 Nov 2020 14:20:20 -0800	[thread overview]
Message-ID: <20201102222020.GA1904687@google.com> (raw)
In-Reply-To: <4f1a1bab-7ac7-b8dd-acb2-6aeb04be3171@jeffhostetler.com>

On 2020.10.30 11:54, Jeff Hostetler wrote:
> 
> 
> On 10/29/20 5:32 PM, Josh Steadmon wrote:
> > In order to more easily debug remote operations, it is useful to be able
> > to inspect both client-side and server-side traces. This series allows
> > clients to record the server's trace2 session ID, and vice versa, by
> > advertising the SID in a new "trace2-sid" protocol capability.
> > 
> 
> Very nice!  This should be very helpful when matching up client and
> server commands.
> 
> 
> > Two questions in particular for reviewers:
> > 
> > 1) Is trace2/tr2_sid.h intended to be visible to the rest of the code,
> >    or is the trace2/ directory supposed to be opaque implementation
> >    detail? If the latter, would it be acceptable to move tr2_sid_get()
> >    into trace2.h?
> 
> I put all the trace2-private stuff in that sub-directory and gave
> everything tr2_ prefixes to hide the details as much as I could
> (and as an alternative to the usual tendency of having everything
> be static within a massive .c file).
> 
> So, yeah, my intent was to make all of it opaque.
> So that just trace2.h contains the official API.
> 
> Perhaps in trace2.c you could create:
> 
> const char *trace2_session_id(void)
> {
>     return tr2_sid_get();
> }

Done in V2, thanks.


> > 2) upload-pack generally takes configuration via flags rather than
> >    gitconfig. From offline discussions, it sounds like this is an
> >    intentional choice to limit potential vulnerability from malicious
> >    configs in local repositories accessed via the file:// URL scheme. Is
> >    it reasonable to load the trace2.announceSID option from config files
> >    in upload-pack, or should this be changed to a flag?
> 
> I don't have the history to comment on this.
> 
> One thing to consider is that the SID for a Git process is built up
> from the SID of the parent and the unique data for the current process.
> So for example, if `git fetch` has SID `<sid1>` and it spawns
> `git upload-pack`, the child will have SID `<sid1>/<sid2>` and if that
> spawns `git index-pack`, that child will have `<sid1>/<sid2>/<sid3>`.
> This is very helpful when tracking down parent/child relationships
> and perf hunting.
> 
> This SID inheritance is passed via an environment variable to
> the child, which extends it and passes the longer version to its
> children.
> 
> So the value being passed between client and server over the
> protocol may look like `<sid1>/<sid2>/<sid3>` rather than just a
> single `<sid_x>` term.  For your purposes, do you want or care if
> you get the single term or the full SID ?

I'm not sure we care too much one way or the other. A single component
of the SID should be enough to join client & server logs, but it's
easier to just send the whole thing.


> Also, there's nothing to stop someone from seeding that environment
> variable in their shell with some mischief before launching the
> top-level Git command.  So the above example might see the SID as
> `<mischief>/<sid1>/<sid2>/<sid3>`.  I'm not sure if this could be
> abused when inserted into the V0/V1/V2 protocol or your logging
> facility.
> 
>     $ GIT_TRACE2_EVENT=1 GIT_TRACE2_PARENT_SID=hello git version
>     {"event":"version","sid":"hello/20201030T154143.660608Z-H86606a97-
> P00001d30",...}
>     ...
> 
> So maybe we want to have a public API to return a pointer to just
> the final `<sid_x>` term ?  (Then again, maybe I worry too much.)

Yeah, it's certainly possible to muck with the SID as you describe, but
I'm not sure I see any big problems that could be caused. If someone
points out an issue I've missed, I'll be happy to change this, though.

> Thanks,
> Jeff

Thanks for the review!


> > Josh Steadmon (10):
> >    docs: new capability to advertise trace2 SIDs
> >    docs: new trace2.advertiseSID option
> >    upload-pack: advertise trace2 SID in v0 capabilities
> >    receive-pack: advertise trace2 SID in v0 capabilities
> >    serve: advertise trace2 SID in v2 capabilities
> >    transport: log received server trace2 SID
> >    fetch-pack: advertise trace2 SID in capabilities
> >    upload-pack, serve: log received client trace2 SID
> >    send-pack: advertise trace2 SID in capabilities
> >    receive-pack: log received client trace2 SID
> > 
> >   Documentation/config/trace2.txt               |  4 +
> >   .../technical/protocol-capabilities.txt       | 13 ++-
> >   Documentation/technical/protocol-v2.txt       |  9 +++
> >   builtin/receive-pack.c                        | 16 ++++
> >   fetch-pack.c                                  | 11 +++
> >   send-pack.c                                   |  9 +++
> >   serve.c                                       | 19 +++++
> >   t/t5705-trace2-sid-in-capabilities.sh         | 79 +++++++++++++++++++
> >   transport.c                                   | 10 +++
> >   upload-pack.c                                 | 23 +++++-
> >   10 files changed, 190 insertions(+), 3 deletions(-)
> >   create mode 100755 t/t5705-trace2-sid-in-capabilities.sh
> > 

  reply	other threads:[~2020-11-02 22:20 UTC|newest]

Thread overview: 66+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-10-29 21:32 [PATCH 00/10] Advertise trace2 SID in protocol capabilities Josh Steadmon
2020-10-29 21:32 ` [PATCH 01/10] docs: new capability to advertise trace2 SIDs Josh Steadmon
2020-10-29 21:32 ` [PATCH 02/10] docs: new trace2.advertiseSID option Josh Steadmon
2020-10-29 21:32 ` [PATCH 03/10] upload-pack: advertise trace2 SID in v0 capabilities Josh Steadmon
2020-10-29 21:32 ` [PATCH 04/10] receive-pack: " Josh Steadmon
2020-10-29 21:32 ` [PATCH 05/10] serve: advertise trace2 SID in v2 capabilities Josh Steadmon
2020-10-29 21:32 ` [PATCH 06/10] transport: log received server trace2 SID Josh Steadmon
2020-10-29 21:32 ` [PATCH 07/10] fetch-pack: advertise trace2 SID in capabilities Josh Steadmon
2020-10-29 21:32 ` [PATCH 08/10] upload-pack, serve: log received client trace2 SID Josh Steadmon
2020-10-29 21:32 ` [PATCH 09/10] send-pack: advertise trace2 SID in capabilities Josh Steadmon
2020-10-29 21:32 ` [PATCH 10/10] receive-pack: log received client trace2 SID Josh Steadmon
2020-10-30 15:54 ` [PATCH 00/10] Advertise trace2 SID in protocol capabilities Jeff Hostetler
2020-11-02 22:20   ` Josh Steadmon [this message]
2020-11-03 21:22     ` Junio C Hamano
2020-11-05 21:01       ` Jeff Hostetler
2020-11-10 21:37       ` Josh Steadmon
2020-10-30 22:31 ` Junio C Hamano
2020-11-02 22:30 ` [PATCH v2 00/11] " Josh Steadmon
2020-11-02 22:30   ` [PATCH v2 01/11] docs: new capability to advertise trace2 SIDs Josh Steadmon
2020-11-03 21:33     ` Junio C Hamano
2020-11-05 21:00       ` Jeff Hostetler
2020-11-12 14:05         ` Ævar Arnfjörð Bjarmason
2020-11-12 17:32           ` Junio C Hamano
2020-11-12 22:10             ` Jeff Hostetler
2020-11-11 22:40       ` Josh Steadmon
2020-11-02 22:31   ` [PATCH v2 02/11] docs: new trace2.advertiseSID option Josh Steadmon
2020-11-03 21:42     ` Junio C Hamano
2020-11-05 19:28       ` Josh Steadmon
2020-11-05 21:24         ` Junio C Hamano
2020-11-06 11:57           ` Johannes Schindelin
2020-11-02 22:31   ` [PATCH v2 03/11] trace2: add a public function for getting the SID Josh Steadmon
2020-11-02 22:31   ` [PATCH v2 04/11] upload-pack: advertise trace2 SID in v0 capabilities Josh Steadmon
2020-11-03 21:48     ` Junio C Hamano
2020-11-05 18:44       ` Josh Steadmon
2020-11-02 22:31   ` [PATCH v2 05/11] receive-pack: " Josh Steadmon
2020-11-02 22:31   ` [PATCH v2 06/11] serve: advertise trace2 SID in v2 capabilities Josh Steadmon
2020-11-04 21:11     ` Junio C Hamano
2020-11-02 22:31   ` [PATCH v2 07/11] transport: log received server trace2 SID Josh Steadmon
2020-11-04 21:14     ` Junio C Hamano
2020-11-11 22:53       ` Josh Steadmon
2020-11-12 21:30         ` Jeff Hostetler
2020-11-02 22:31   ` [PATCH v2 08/11] fetch-pack: advertise trace2 SID in capabilities Josh Steadmon
2020-11-04 21:22     ` Junio C Hamano
2020-11-05 18:58       ` Josh Steadmon
2020-11-05 19:21         ` Junio C Hamano
2020-11-11 23:32           ` Josh Steadmon
2020-11-02 22:31   ` [PATCH v2 09/11] upload-pack, serve: log received client trace2 SID Josh Steadmon
2020-11-04 21:26     ` Junio C Hamano
2020-11-05 19:12       ` Josh Steadmon
2020-11-02 22:31   ` [PATCH v2 10/11] send-pack: advertise trace2 SID in capabilities Josh Steadmon
2020-11-02 22:31   ` [PATCH v2 11/11] receive-pack: log received client trace2 SID Josh Steadmon
2020-11-03  1:02   ` [PATCH v2 00/11] Advertise trace2 SID in protocol capabilities Junio C Hamano
2020-11-11 23:29 ` [PATCH v3 00/11] Advertise session ID " Josh Steadmon
2020-11-11 23:29   ` [PATCH v3 01/11] docs: new capability to advertise session IDs Josh Steadmon
2020-11-11 23:29   ` [PATCH v3 02/11] docs: new transfer.advertiseSID option Josh Steadmon
2020-11-11 23:29   ` [PATCH v3 03/11] trace2: add a public function for getting the SID Josh Steadmon
2020-11-11 23:29   ` [PATCH v3 04/11] upload-pack: advertise session ID in v0 capabilities Josh Steadmon
2020-11-11 23:29   ` [PATCH v3 05/11] receive-pack: " Josh Steadmon
2020-11-11 23:29   ` [PATCH v3 06/11] serve: advertise session ID in v2 capabilities Josh Steadmon
2020-11-11 23:29   ` [PATCH v3 07/11] transport: log received server session ID Josh Steadmon
2020-11-11 23:29   ` [PATCH v3 08/11] fetch-pack: advertise session ID in capabilities Josh Steadmon
2020-11-11 23:29   ` [PATCH v3 09/11] upload-pack, serve: log received client session ID Josh Steadmon
2020-11-11 23:29   ` [PATCH v3 10/11] send-pack: advertise session ID in capabilities Josh Steadmon
2020-11-11 23:29   ` [PATCH v3 11/11] receive-pack: log received client session ID Josh Steadmon
2020-11-25 22:08   ` [PATCH v3 00/11] Advertise session ID in protocol capabilities Junio C Hamano
2020-11-25 22:56     ` Josh Steadmon

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:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

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

  git send-email \
    --in-reply-to=20201102222020.GA1904687@google.com \
    --to=steadmon@google.com \
    --cc=git@jeffhostetler.com \
    --cc=git@vger.kernel.org \
    --cc=jeffhost@microsoft.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* 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.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

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).