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

Andrew Tridgell tridge at osdl.org
Wed May 18 00:24:41 GMT 2005


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. 

Some comments from briefly reading the code:

 - a bit of whitespace between functions would be nice, perhaps with a
   small comment

 - I don't see the point of the POPT_COMMON_NET stuff. The
   POPT_COMMON_* popt tables are for command line options shared
   between programs. I don't see why these options would be used
   outside of the net command, so they are probably better kept in the
   utils/net/net.c code rather than in the generic popt code.

 - 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?

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.

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?

The use of unions to provide a single interface to a group of
functions that perform the same type of operation was introduced in
includes/smb_interfaces.h as it made code cleaner and easier to read,
allowing us to have a single smb_raw_read() call instead of having 4
separate calls, one for each type of read in the protocol. That also
greatly simplified the ntvfs layer.

The reason it made sense in that case was that the SMB protocols
defines so many ways of doing essentially the same thing, and I wanted
to hide some of that protocol detail in the structure of our client
and server code. I think it worked quite well.

This doesn't means unions need to be used absolutely everywhere
though. There needs to be a reason for using them, otherwise they just
make the code less rather than more readable.

So if we really are grouping together multiple operations into one
"class" of operation, use the union/level method to distinguish which
method the caller wants, but if there is only one operation then just
use a simple structure.

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.

Cheers, Tridge


More information about the samba-technical mailing list