Conversation
kolesnikovae
left a comment
There was a problem hiding this comment.
Looks good! There's one minor issue with the parser error check – please take a look
| buf := bytes.NewBuffer(nil) | ||
| if _, err := io.CopyN(buf, conn, int64(header.Size-headerSize)); err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| if err := binary.Read(buf, binary.LittleEndian, &resp.ProcessID); err != nil { | ||
| return nil, fmt.Errorf("unable to read process ID: %w", err) | ||
| } | ||
|
|
||
| if err := binary.Read(buf, binary.LittleEndian, &resp.GUID); err != nil { | ||
| return nil, fmt.Errorf("unable to read process ID: %w", err) | ||
| } | ||
|
|
||
| // now parse the strings out | ||
| p := &nettrace.Parser{Buffer: buf} | ||
| p.UTF16NTS() | ||
| resp.CommandLine = p.UTF16NTS() | ||
| p.UTF16NTS() | ||
| resp.OS = p.UTF16NTS() | ||
| p.UTF16NTS() | ||
| resp.Arch = p.UTF16NTS() | ||
| p.UTF16NTS() | ||
| resp.AssemblyName = p.UTF16NTS() | ||
| p.UTF16NTS() | ||
| resp.RuntimeVersion = p.UTF16NTS() |
There was a problem hiding this comment.
We need to check the parser error with p.Err() in the end
nit:
It looks like the buffer size is known in advance, probably we should pre-alloc it. Also, for small payloads of known size io.ReadAtLeast might be a better alternative to io.Copy. Provided that we will be calling ProcessInfo2 relatively frequently, this micro-optimisation might make sense. Also, we could parse all fields with parser, like:
md := Metadata{p: &Parser{Buffer: blob.Payload}}
md.p.Read(&md.Header.MetaDataID)
md.Header.ProviderName = md.p.UTF16NTS()
md.p.Read(&md.Header.EventID)
md.Header.EventName = md.p.UTF16NTS()
md.p.Read(&md.Header.Keywords)
md.p.Read(&md.Header.Version)
md.p.Read(&md.Header.Level)There's probably a bug in the UTF16NTS() function, as it should be called once per a string:
- It looks like the fallback path (
decodeUTF16NTS) only works as intended if the very first char is a non-ASCII one - the function is quite wasteful: we probably should reuse bytes buffer / use
strings.Buidler
| if header.CommandSet != CommandSetServer || header.CommandID != 00 { | ||
| return nil, fmt.Errorf("unexpected response header: commandSet=%v (expected 0xff) commandID=%v (expected 0x00)", header.CommandSet, header.CommandID) | ||
| } |
There was a problem hiding this comment.
It would be great if we could get the error code when CommandID is 0xFF
In readResponse we have quite weak check, and a TODO note – I'd fix this:
func verifyResponseHeader(h Header) error {
if h.CommandSet != CommandSetServer {
return fmt.Errorf("%w: received response with unknown command set %#x", ErrDiagnosticServer, h.CommandSet)
}
switch h.CommandID {
default:
return fmt.Errorf("%w: received response with unknown command ID %#x", ErrDiagnosticServer, h.CommandID)
case 0x00:
return nil
case 0xFF:
var er ErrorResponse
if err := binary.Read(r, binary.LittleEndian, &er); err != nil {
return err
}
return fmt.Errorf("%w: error code %#x", ErrDiagnosticServer, er.Code)
}
}
This implements the ProcessInfo2 call, to figure out more information about the process and runtime