On Wed, Sep 07, 2016 at 12:31:29PM +0200, Günther Deschner wrote:
> On 07/09/16 02:05, Jeremy Allison wrote:
> > On Tue, Sep 06, 2016 at 10:38:37AM +0200, Günther Deschner wrote:
> >> On 05/08/16 02:01, Jeremy Allison wrote:
> >>> On Wed, Jul 27, 2016 at 05:20:36PM +0200, Günther Deschner wrote:
> >>>> Hi,
> >>>>
> >>>> attached some more patches for the cluster management protocol testing.
> >>>>
> >>>> Please review & push,
> >>>
> >>> Mostly looks good - just a few immediate comments :
> >>>
> >>> In [PATCH 04/13] s4-torture: add new tests for clusapi resourcetypes.
> >>>
> >>> + if (W_ERROR_EQUAL(r.out.result, WERR_MORE_DATA)) {
> >>> + r.out.lpOutBuffer = talloc_zero_array(tctx, uint8_t, *r.out.lpcbRequired);
> >>> + r.in.nOutBufferSize = *r.out.lpcbRequired;
> >>> + torture_assert_ntstatus_ok(tctx,
> >>> + dcerpc_clusapi_ResourceTypeControl_r(b, tctx, &r),
> >>> + "ResourceTypeControl failed");
> >>> + }
> >>>
> >>> + r.in.nOutBufferSize = 0x400;
> >>> + r.out.lpOutBuffer = talloc_zero_array(tctx, uint8_t, r.in.nOutBufferSize);
> >>> +
> >>> + torture_assert_ntstatus_ok(tctx,
> >>> + dcerpc_clusapi_ResourceTypeControl_r(b, tctx, &r),
> >>> + "ResourceTypeControl failed");
> >>>
> >>> Shouldn't you be checking for NULL after the talloc's here,
> >>> or are you just expecting the dcerpc_clusapi_ResourceTypeControl_r()
> >>> calls to catch it ?
> >>
> >> Basically yes, in torture tests it is a not so un-common practice not to
> >> explicitly check for the success of talloc (which I personally find
> >> acceptable of course only in torture tests). If you insist I can fix it
> >> here but then we need to have a very large followup patch for all other
> >> testsuites as well.
> >
> > Fair enough. Can you resend the patchset for re-review, it got dropped
> > out of my inbox (sorry).
>
> Sure, here you go.
LGTM - pushed - thanks !