PATCHv5: adjust 'net ads add keytab' for windows SPN(s) & add new 'net ads setspn' subcommand

[Andreas Schneider]

On Wednesday, 28 February 2018 20:22:10 CET Jeremy Allison wrote:
> On Wed, Feb 28, 2018 at 11:36:13AM +0000, Noel Power via samba-technical 
wrote:
> > Thanks Jeremy for having a look, I have adjusted the patch
> > 
> > summary of changes:
> >   * bulk 'principle' -> 'principal'
> >   * patch2: redundant else clause removed
> >   * patch3: missing talloc return checks added
> >  
> > 
> >   * patch4:
> > >> +
> > >> +static bool ads_set_machine_account_spns(TALLOC_CTX *ctx,
> > > 
> > > Return ADS_STATUS here, otherwise you're losing a
> > > error code that might be useful higher up.
> > 
> > This is a local static function, the only caller (in this module)
> > doesn't return an ADS_STATUS so it will never be passed up anywhere so I
> > didn't change it, hope that is ok.  Do you still want me to change it ?
> > 
> >   * patch5: redundant talloc_stackframe return checks removed,
> > 'ads_setspn_list' return changed to 'bool'
> >   * patch6: redundant talloc_stackframe return checks removed,
> > 'ads_setspn_add' return changed to 'bool'
> > 
> > > ads_setspn_add returns either 0 or -1. Why isn't this
> > > a bool ?
> > 
> > For consistency I just followed the existing code where presumably for
> > simplicity the implementation functions called by cmdline processing
> > (int (*fn)(struct net_context *c, int argc, const char **argv) handler
> > generally call a helper function with the same return type so they can
> > pass back the results without conversion.
> > 
> > Anyway I've changed all the setspn functions to use bool now
> > 
> > > I'm not 100% convinced on the |= idiom for
> > > a return value (I know the ldb code uses it a lot).
> > > 
> > > Only a nit-pick here really...
> > > 
> > >> +		ret |= ads_setspn_add(ads, argv[0], argv[1]);
> > >> +	} else {
> > >> +		ret |= ads_setspn_add(ads, lp_netbios_name(), argv[0]);
> > >> +	}
> > >> +
> > >> +done:
> > >> +	if (ads) {
> > >> +		ads_destroy(&ads);
> > >> +	}
> > >> +	return ret;
> > >> +}
> > 
> > I've removed these (for setspn subcommands) also
> > 
> >   * patch7: redundant talloc_stackframe return checks removed,
> > 'ads_setspn_delete' return changed to 'bool'
> 
> Thanks for all your hard work Noel:
> 
> Reviewed-by: Jeremy Allison <jra at samba.org>
> 
> Andreas, can you do a quick re-review and then
> I can push for Noel if you're happy ?

I've pushed it to autobuild with some minor fixes:

DEBUG(3) -> DBG_INFO
int -> size_t in for loops
bool ret -> bool ok


	Andreas


-- 
Andreas Schneider                   GPG-ID: CC014E3D
Samba Team                             asn at samba.org
www.samba.org