Re: [RFC PATCH 20/27] coco/tdx-host: Add connect()/disconnect() handlers prototype

[<dan.j.williams@intel.com>]

Xu Yilun wrote:
[..]
> IOW, I like this current piece of code cause it is in perfect balance.
> I don't have to change the mindset much for code design. I get the benifit
> of auto-cleanup, and the local cleanup handlers (tlink_spdm, tlink_ide)
> are cheap but clearly tell me what will happen if any step fails.

I think in this case of conflicting minor preferences the tie goes to
the submitter. While I personally think the discipline of clearly
delineating objects and ownerships yields maintainability benefits, I
also do not hate the model of "extend existing object with scope based
setup".

So,

Acked-by: Dan Williams <dan.j.williams@intel.com>

> > > 	if (IS_ERR(tlink_spdm)) {
> > > 		pci_err(pdev, "fail to setup spdm session\n");
> > > 		return PTR_ERR(tlink_spdm);
> > > 	}
> > > 
> > > 	struct tdx_link *tlink_ide __free(tdx_ide_stream_teardown) =
> > > 		tdx_ide_stream_setup(tlink);
> > > 	if (IS_ERR(tlink_ide)) {
> > > 		pci_err(pdev, "fail to setup ide stream\n");
> > > 		return PTR_ERR(tlink_ide);
> > > 	}
> > 
> > No strict need for scope-based cleanup if this is the last resource
> > acquisition,
> 
> So if we don't do auto-cleanup for the last one, do we still need a
> structure for that? If not,
> 
>  struct tdx_link {
> 	struct tdx_spdm *spdm;
> 	
> 	int ide_stream_field1;
> 	int ide_stream_field2;
> 	...
>  }
> 
> seems so wierd.

Yeah, a little clunky.

> > but maybe there are other PCI/TSM core things to do that
> > are not shown.
> 
> There is no following items for now but I think cleanup for the last one
> is good. Otherwise we may face with the same problem as goto, that we
> see unrelated changes (add cleanup for previous one) when we add a new
> step.

Again, this is a case of I still disagree with shipping the dead code,
but not enough to NAK your preference.