[PATCH] smbcacls added support for a -I flag

[Volker Lendecke]

On Sun, May 02, 2010 at 12:50:24PM -0700, matthew at mcgillis.org wrote:
> When I added the inherit code I struggled with a number of issues with this code:
> 
> 1) The parameters are poorly processed and can create some
> undesired side effects if you are not careful about how
> you call this. (I'm focused on only providing the
> functionality my customer desires so not that interested
> in fixing this)
> 2) The code has a number of duplicate chunks of code that
> if consolidated would help prevent bugs. (I'm not focused
> on this unless it overlapped with the code I generated for
> inherit)
> 3) To me smbcacls should do all its work through
> libsmbclient although it isn't actually clear to me this
> is 100% possible. (not a requirement for my customer)
> 
> At a code structural level the main key parts of the original code is:
> 
> owner_set(struct cli_state *cli, enum chown_mode change_mode,
>                      const char *filename, const char *new_username)
> cacl_set(struct cli_state *cli, char *filename, char *the_acl, enum acl_mode mode)
> cacl_dump(struct cli_state *cli, char *filename)
> 
> From the original code const char *filename was only used
> once. For consistency through out the code I opted to use
> the non const since the originally code seemed to lean a
> little more towards non const rather than const. It really
> makes no difference to me which is actually decided on. I
> was only interested in consistency.
> 
> In the above procedures we generally do the following:
> 
> 1) Grab a current SEC_DESC
> 2) Do something based on that SEC_DESC
> 3) Set a SEC_DESC back on target object (cacl_dump doesn't do this step)
> 
> All of the procedures do step 1 slightly differently. When
> adding inherit I also have to do the same three basic
> steps. I'm not fond of cut and paste code so I opted to
> consolidate all of step 1 and step 3 into common
> procedures:
> 
> get_secdesc(struct cli_state *cli, char *filename);
> 
> I then made the necessary changes for owner_set, cacl_set,
> cacl_dump and inherit to use this common get_secdesc.
> 
> Similarly I created
> 
> set_secdesc(struct cli_state *cli, char *filename SEC_DESC *sd)
> 
> And again made changes necessary for owner_set, cacl_set
> and inhert to use this common code.
> 
> I hope that helps explain the choices I made. Let me know
> how you would like me to proceed. To get you what you
> desire.

Yes, it does. Thanks for the explanation. All that was not
obvious to me from the patch.

While the existing code in smbcacls might be a very bad
example, we tend to use const wherever it is sanely
possible. It is very often difficult in C to get that right,
but just passing a file name through to cli_ntcreate() is
definitely one of those cases where a const char * is
appropriate.

Your restructuring makes a lot of sense, and here git really
shines. It is extremely easy in git to create a series of
small checkins that do individual, easily explainable
changes. It would be great if your changes that you just
explained would be represented in a series of small patches.

Would that be possible?

Thanks,

Volker
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 197 bytes
Desc: Digital signature
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20100502/bb2620d9/attachment.pgp>