[PATCH] smbcacls added support for a -I flag

[matthew at mcgillis.org]

> One real requestion though: Your patch contains changes to
> for example cacl_dump, to owner_set etc. Some of those seem
> to be pure reformattings, but to me that seems a bit
> difficult to really review for the real changes. For example
> there's a hunk in your patch that says
> 
> static int owner_set(struct cli_state *cli, enum chown_mode change_mode, 
> -                       const char *filename, const char *new_username)
> +                    char *filename, const char *new_username)
> {
> 
> You've removed the const here. To me it seems that this was
> required because the routine get_fileinfo() does not have
> that const. Can you change the patch to add the const to
> get_fileinfo instead of removing it from owner_set? I would
> like to see a patch that contains almost only + and almost
> no - if possible.

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.

Thanks,
Matthew