Implemented and Tested ReadPublic

[matt-tsai]

Implemented and Tested ReadPublic

[chrisfenner]

Thanks for the change! This looks pretty solid, I just have some minor notes (mostly about the test)

[chrisfenner]

Can we defer a FlushContext similar to what we do here:

go-tpm/direct/tpm2/policy_test.go

Lines 53 to 61 in 7cd0fbd

	cleanup := func() {
	t.Helper()
	flush := FlushContext{
	FlushHandle: rsp.ObjectHandle,
	}
	if err := flush.Execute(thetpm); err != nil {
	t.Errorf("could not flush signing key: %v", err)
	}
	}

Something like:

defer func() { 
 	flush := FlushContext{ 
 		FlushHandle: rsp.ObjectHandle, 
 	} 
 	if err := flush.Execute(thetpm); err != nil { 
 		t.Errorf("could not flush signing key: %v", err) 
 	} 
 }()

[alexmwu]

@chrisfenner
Sorry to make this more convoluted. Common commands like FlushContext take up more lines of code than go-tpm/tpm2 currently does. Users likely wouldn't want the clutter of management commands like FlushContext taking too much space; they care about the business logic of what they're putting into commands like CreatePrimary or ReadPublic.

Would having the idiom below be easier to read?

defer func() {
  if err := FlushContext{FlushHandle: rsp.ObjectHandle}.Execute(thetpm); err != nil {
    t.Errorf("...")
  }
}

we could also make wrappers for commands like FlushContext, although I think it seems unnecessary for a small command like FlushContext.
e.g. in a util pkg or something:

direct/tpm2/directutil/context
func FlushContext(t transport.TPM, flushHandle handle) error {
  return FlushContext{ FlushHandle: flushHandle}.Execute(t)
}

direct/tpm2/read_public_test.go:
defer func() {
  if err := directutil.FlushContext(thetpm, rsp.ObjectHandle); err != nil {
    t.Errorf("...")
  }
}

[chrisfenner]

We discussed offline - we don't have to assert that every flush succeeded during the tests (the current tests don't; we just need a FlushContext test that checks it once).

So, I'd like to update my suggestion to just: can we add

defer &FlushContext{rsp.ObjectHandle}.Execute(thetpm))

here?

[matt-tsai]

I was getting
function must be invoked in defer statement for

defer &FlushContext{rsp.ObjectHandle}.Execute(thetpm))

So my work around was

flushContext := FlushContext{FlushHandle: rspCP.ObjectHandle}
defer flushContext.Execute(thetpm)

which seems to work just as well

[alexmwu]

Can you try defer (&FlushContext{rsp.ObjectHandle}).Execute(thetpm))

[matt-tsai]

The same error occurs.

[chrisfenner]

I'm fine with the 2 line version here, and if we think of a neat idiom later it's easy to change.

[alexmwu]

@chrisfenner
Sorry to make this more convoluted. Common commands like FlushContext take up more lines of code than go-tpm/tpm2 currently does. Users likely wouldn't want the clutter of management commands like FlushContext taking too much space; they care about the business logic of what they're putting into commands like CreatePrimary or ReadPublic.

Would having the idiom below be easier to read?

defer func() {
  if err := FlushContext{FlushHandle: rsp.ObjectHandle}.Execute(thetpm); err != nil {
    t.Errorf("...")
  }
}

we could also make wrappers for commands like FlushContext, although I think it seems unnecessary for a small command like FlushContext.
e.g. in a util pkg or something:

direct/tpm2/directutil/context
func FlushContext(t transport.TPM, flushHandle handle) error {
  return FlushContext{ FlushHandle: flushHandle}.Execute(t)
}

direct/tpm2/read_public_test.go:
defer func() {
  if err := directutil.FlushContext(thetpm, rsp.ObjectHandle); err != nil {
    t.Errorf("...")
  }
}

[chrisfenner]

Not at all @alexmwu, I totally agree with the feedback. It would be awesome to have a one-line idiom for "defer flush this thing" and even awesomer to have a test version that asserts that the flush succeeded.

I'm in email/meeting mode all day today so I can't remember if this would compile but can we have something along the lines of

defer t.Run("🚽", &FlushContext{rsp.ObjectHandle}.Execute(thetpm) == nil)

in the tests and

defer &FlushContext{rsp.ObjectHandle}.Execute(thetpm))

in sample code?

If that's not possible with the current reflection logic, are there tweaks we could make to make it possible?

[chrisfenner]

I'm fine with the 2 line version here, and if we think of a neat idiom later it's easy to change.