net command: list shares, add share, del share + net options

[Rafal Szczesniak]

On Wed, May 18, 2005 at 10:24:41AM +1000, Andrew Tridgell wrote:
> Grégory,
> 
> Thanks for this! The maintainer of the libnet code is mimir, so he
> should look at it for you and apply it if he thinks its OK.
> 
> Ideally the code would provide an async interface, using the composite
> function structures. I don't mind if mimir wants to apply it now
> without that, and look at converting to an async interface later. 

Quite possible, but the code (still reviewing it) needs couple of
changes anyway.

> Some comments from briefly reading the code:
> 
>  - a bit of whitespace between functions would be nice, perhaps with a
>    small comment

Agreed.

>  - the use of unions for these particular libnet commands doesn't seem
>    to add anything. I know we use the union/level system a lot in
>    Samba4, but it is meant for cases where we want to offer multiple
>    ways of doing an operation (different levels of operation). The way
>    you have structured it, there is only one method, so the two union
>    levels just server to confuse the code. For example, you have:
> 
>      union libnet_DelShare {
> 	struct {
> 		enum libnet_DelShare_level level;
> 		struct _libnet_DelShare_in {
> 			const char *server_name;
> 			const char *share_name;
> 		} in;
> 		struct _libnet_DelShare_out {
> 			const char *error_string;
> 		} out;
> 	} generic;
> 	struct {
> 		enum libnet_DelShare_level level;
> 		struct _libnet_DelShare_in in;
> 		struct _libnet_DelShare_out out;
> 	} srvsvc;
>      };
> 
> so srvsvc and generic are identical structures. If there was a 3rd
> method which was different then you could have use "generic, srvsvc;"
> on the first structure so make this simpler, but given there is only
> one method, I'd suggest this:
> 
> 	struct libnet_DelShare {
> 		struct {
> 			const char *server_name;
> 			const char *share_name;
> 		} in;
> 		struct {
> 			const char *error_string;
> 		} out;
> 	};
> 
> which makes the code considerably simpler. We don't need to force
> unions where they aren't needed.
> 
> Mimir, looking at the current libnet code, I notice quite a few places
> where unions seem to have been used gratuitously. Is there any
> particular reason for that?

No. It's 'legacy' code that's meant to be replaced. It was there yet
before we started talking about turning libnet into asynchronous library
using composite functions. Most of those unions (and functions that use 
them) are waiting to be superseded by composite asynchronous routines
using already known way of passing input and output.

> Where there really is more than one way we want to do things, and
> those different methods need to take different parameters, then a
> union/level makes sense. For example, in libnet_passwd.h the
> samr_handle method and the other methods take quite different
> parameters, so this would make sense:
> 
>     union libnet_SetPassword {
> 	struct {
> 		enum libnet_SetPassword_level level;
> 		struct {
> 			const char *account_name;
> 			const char *domain_name;
> 			const char *newpassword;
> 		} in;
> 		struct {
> 			const char *error_string;
> 		} out;
> 	} generic, samr, krb5, ldap, rap;
> 
> 	struct {
> 		enum libnet_SetPassword_level level;
> 		struct {
> 			const char           *account_name;
> 			struct policy_handle *user_handle;
> 			struct dcerpc_pipe   *dcerpc_pipe;
> 			const char           *newpassword;
> 		} in;
> 		struct {
> 			const char *error_string;
> 		} out;
> 	} samr_handle;
>     }
> 
> notice that I have removed the "_libnet_SetPassword_out" stuff, as it
> doesn't add anything and just clutters up the header. Also notice that
> I have collapsed the samr, krb5, ldap and rap level structures into
> the same structure definition as the generic one. They are exactly the
> same, so having them separately defined just makes for longer, less
> readable, header files.

Absolutely agreed.

> but if we look at something like "union libnet_RemoteTOD" in
> libnet_time.h then the union seems quite gratuitous. We only offer one
> level, so why not just make it a simple struct and do away with all
> the complicated calling conventions?

I believe many of those definitions look like this because they are
based on similiar pattern which is probably the first libnet function
that have appeared in the code.

[...]
> If there are multiple methods but they all take the same parameters,
> then certanly using a union/level is fine, but use the "a,b,c;" syntax
> to define the various elements, rather than defining the structures in
> the union 3 times. That makes it obvious to anyone reading the code
> that the structures really are the same, without having to carefully
> compare the components.

That's right. Thanks for reminding me that. That's another task to do
(to simplify the structures) apart from converting functions to
asynchronous.


cheers,
-- 
Rafal Szczesniak
Samba Team member  http://www.samba.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
Url : http://lists.samba.org/archive/samba-technical/attachments/20050518/8eb2dd0c/attachment.bin