[illumos-Developer] [REVIEW] 1120 Use real file descriptors for smbfs named pipes.

[Gordon Ross]

On Fri, Jun 17, 2011 at 12:41 PM, Eric Schrock <eric.schrock at delphix.com> wrote:
> As usual with SMB, I understand just about nothing about the meat of what's
> going on here, and I've tried to be as thorough as possible, but the result
> is mostly style nits (some of which are clearly out of scope and more just
> observations).  Feel free to push back on anything you don't agree with.
> - Eric

OK.  I'll go ahead and explain the style situation first.

The history is that, when this project integrated into ON
(back in build 84 as I reacall?) we were assuming that
we might try to track "up stream" changes from Apple.
So the whole thing "got a pass" on style issues, keeping
the Apple or BSD style of the original files most places.

Then, it turned out lots more work was needed in parts of
this code, so some of it was basically re-written.  Along
the way, people from the server team asked if I could
try to reduce name conflicts with smb_* symbols in the
server code, so for things exposed there, I started using
smbfs_*, or at least most of the time.  But I've resisted
suggestions to go on a renaming expedition, as I don't
think that's really worth the time.

So, I try make new additions reasonably clean, and
do some cleanup of old stuff when it's convenient.

> usr/src/cmd/fs.d/smbclnt/Makefile
> 36: Do we really need to build and install these tests as part of every
> build instead of by request?

No, but others have strongly suggested that tests should
be maintained in the tree, and compiled (to avoid bit rot:)
There are installed in the proto area, but not packaged.
Is that OK?

> usr/src/cmd/fs.d/smbclnt/test/Makefile
> 44-47: If this genuinely useful, is there a way to key it off some
> environment variable or alternate target instead of having to edit the
> Makefile?

Dunno.  I've left that lots of places, which probably will
annoy some people.  As far as I can tell, I'm one of only
a few who use dbx.  (No, mdb is not a substitute;)

> 51: Particularly if we're going to be building and installing this all the
> time, can we put this somewhere more descriptive, like /usr/lib/smbsrv/test?

Well, these are part of the SMB client.  They are client side test that
contact the "srvsvc" RPC service (two sub functions: info, enum).

>  Or at least name the programs with 'test' in the name?

Sure, it started life as "nptest" but I thought that name sucked :)

Actually, I should probably combine them into one.
I was just being quick there...

> usr/src/cmd/fs.d/smbclnt/test/srvenum.c
> It's test code, but:
> 45: s/recv./receive/

You want me to use whole words in my comments?  :)

> 92, 95: Why are these declarations split across lines?  And shouldn't they
> be static?

If it were a library or something, I might care :)
It's a test program.  but... OK.

> 100, 106: static
> 175: What does this mean?  Is this code still used?

This code also serves as an example of how to use
the client-side interface to server named pipes.
If this were one of the commands (smbutil, or
mount_smbfs) then this code would use this
#if 0 section to get $HOME/.nsmbrc settings.
I thought it helpful to have it there for reference.

> usr/src/cmd/fs.d/smbclnt/test/srvinfo.c
> Similar comments to above.

Yeah, it was a copy.  Maybe I'll just delete the
enum one.  One test is enough to cover this.

> usr/src/cmd/mdb/common/modules/nsmb/nsmb.c
> 24: Copyright? (ditto other files)

Well, much of this was from prior work, taken from
cr.opensolaris.org and not changed since, so I only
added a copyright where I actually changed things.

Should I slap a copyright on everything?

> usr/src/cmd/mdb/intel/amd64/smbfs/Makefile
> 42-43: Don't we have C99_ENABLE in Makefile.master?  Ditto other Makefiles.

The default is: (from Makefile.master)
   C99MODE=	$(C99_DISABLE)
which sucks, but would apparently be a big effort
to change globally without breaking stuff.
So I just add this where C99 is needed:
     C99MODE=	$(C99_ENABLE)

> usr/src/lib/libsmbfs/netsmb/smb_keychain.h
> 53: More a comment on this style (which seems pervasive in these headers),
> but normally I'd expect to see "extern" for function prototypes in headers.

Lots of headers don't bother with the extern except for data.
(where it actually matters)

> usr/src/lib/libsmbfs/netsmb/smbfs_api.h
> 64: The "= 0" is superfluous.

Sure, but it's documentation.  These share type values
are actually exposed in the protocol, so it's critically
important that these are numbered as shown.  Maybe
there are better ways to convey that?

> 151-152: Everything else starts with "smb_" but this is "smbfs_", why?
> usr/src/lib/libsmbfs/smb/ctx.c

Explained above (name conflicts in smbsrv code).

> 219: not part of this review, but can you delete this while you're in the
> neighborhood?

OK

> 412-451: free() is defined to be a no-op for NULL to avoid checks like
> these.   Why do we sometimes set the member to NULL and other times not?

This code used too have consumers that provided static storage
for the struct smb_ctx, and could reuse it.  I don't recall if that still
happens anywhere.  These should probably be consistent.
I'll make the new ones consistent.

> 508-510, 521-523: match if() brace with else {} style

OK, though see above about style.
(I don't really want to "re style" all of this code)

> 535, 540, etc: I prefer to see "!= NULL" so I know it's a pointer and not a
> boolean value

OK.

> 592: Weird style to have the "0 ==" first
> usr/src/lib/libsmbfs/smb/file.c

Some places I've worked liked that style, and now and then I
revert to an old habit.  Some find it easier read...

> 70-72: Perhaps I'm missing something in the design here, but I thought part
> of the motivation was to automatically clean up state if a process dies and
> the file descriptors are closed.  If that's the case, why do we need an
> explicit ioctl() separate from the close(2) call?

When I was doing the driver part of this (some time ago) I was
not sure if user-level might want to reuse these handles.  So for
completeness, all ioctls that add state to the driver handle have
complementary ioctls to undo that state.  So then, as I recall, I
wanted that code path to be tested... and never bothered to
remove the "close" ioctl.  It's relatively harmless, but you're
right, it's not necessary here.   I'll remove it.

> 220: It's hard to see system-wide changes in diff view, but
> is ioc_fh still needed?

No, not really.  It was just convenient to leave it there.
The driver still supports passing in the fid (optional).
Passing in the -1 tells the driver to use the FID it
has stored for this driver handle.

> usr/src/lib/libsmbfs/smb/print.c
> 103: Why is this comment floating at the end of the file?

There used to be an smb_close_printer here.
Perhaps it should say: close of print files is
handled by smb_fh_close.

> usr/src/uts/common/fs/smbclnt/netsmb/smb_conn.h
> 305: Another general comment on style beyond the scope of this fix, this
> file seems to mix prototypes with no argument names vs. those with names.

Legacy style (as explained at the top of this message)

> usr/src/uts/common/fs/smbclnt/netsmb/smb_smb.c
> 494: match brace style

ditto style

> 541: missing check for 'error'

Yes... (though an error here is not critical)

> usr/src/uts/common/fs/smbclnt/netsmb/smb_usr.c
> 491, 544: superfluous assignment to 0

OK

> 494, 547: how can 'ioc' be NULL here?

I guess it can't.  Again, this all came from BSD, where
kmem_alloc can fail, and there must have been a
"goto out" on kmem_alloc failure.

I can remove the "if (ioc != NULL)" part if you like.

> usr/src/uts/common/fs/smbclnt/netsmb/subr_mchain.c
> 563: why does this function return an int?

We have code using the "mchain.h" interface that
compiles for either kernel or user level.  In-kernel,
there happen to be no error return cases in here.
In the BSD kernel, and in our user-land, there are.
So while slightly odd here, this lets mchain present
the same interface all places.

Thanks for having a look.
Gordon