9735 Need to provide SMB 2.1 Client (and prerequisites) #37
Conversation
usr/src/cmd/smbsrv/smbd/smbd_logon.c
Outdated
| /* Handle user@domain format. */ | ||
| if (tmp_user.lg_domain[0] == '\0' && | ||
| (p = strchr(tmp_user.lg_e_username, '@')) != NULL) { | ||
| buf = strdup(tmp_user.lg_e_username); |
There was a problem hiding this comment.
Maybe if (buf == NULL) goto errout; ??
There was a problem hiding this comment.
Yeah, checking for strdup failure would be nice. (fixed in the recent push -f)
gwr
force-pushed
the
smb2clnt1f
branch
4 times, most recently
from
4fa7705
to
848c425
Compare
usr/src/common/smbclnt/smbfs_ntacl.c
Outdated
| #else /* _KERNEL */ | ||
|
|
||
| #include <stdio.h> | ||
| #include <stdlib.h> | ||
| #include <strings.h> | ||
|
|
||
| #include <idmap.h> | ||
| #endif /* _KERNEL */ |
There was a problem hiding this comment.
Yeah, probably should update to match the ifdef, here and below.
(Thanks @sjorge)
gwr
force-pushed
the
smb2clnt1f
branch
2 times, most recently
from
c17a751
to
59626d9
Compare
| # Copyright 2017 Nexenta Systems, Inc. All rights reserved. | ||
| # | ||
|
|
||
| set -o emacs |
There was a problem hiding this comment.
I think this probably shouldn't be here. It may override personal preferences of other dbx users...
There was a problem hiding this comment.
(Even though it is my preference)
There was a problem hiding this comment.
agreed. just forgot it was there when I committed this.
| # Do this both ways, for now... | ||
| CSTD= $(CSTD_GNU99) | ||
| C99MODE= -xc99=%all | ||
| C99LMODE= -Xc99=%all |
There was a problem hiding this comment.
This feels wrong somehow. Is this to workaround confusion in upper Makefiles?
There was a problem hiding this comment.
This was just because I was building this code on a couple different maintenance branches both before and after this sweeping "Hey, let's go rename a bunch of variables!" change.
We can get rid of the "old way" part for this integration.
| int | ||
| smb_iod_start(smb_ctx_t *ctx) | ||
| { | ||
|
|
| case AF_NETBIOS: | ||
| err = fknewvc(ctx, ai); | ||
| if (err == 0) | ||
| goto OK; |
There was a problem hiding this comment.
This is the only non-errorneous return condition here. Maybe just return (0) right from this code and skip the OK: branch altogether?
There was a problem hiding this comment.
There used to be some other stuff here so the goto made more sense.
As it is now, yeah, just a break here would be fine.
| } | ||
|
|
||
| /* | ||
| * Resolve the server address, |
There was a problem hiding this comment.
Seems like your comment formatter could reflow these into 80 columns instead of 40ish?
usr/src/common/smbclnt/smbfs_ntacl.c
Outdated
| @@ -61,21 +64,18 @@ | |||
| #define NT_SD_REVISION 1 | |||
| #define NT_ACL_REVISION 2 | |||
|
|
|||
| #ifdef _KERNEL | |||
| #ifdef __lint | |||
There was a problem hiding this comment.
I don't understand the purpose of this.
There was a problem hiding this comment.
Ah I see. Don't we have __unused now? seems like ti would be cleaner than this, which is a really ugly hack.
There was a problem hiding this comment.
I was getting compiler errors with __unused.
I see now that was probably due to the CSTD Makefile change.
I'll try fixing that and then this can probably go back to using
the inline with an __unused arg.
| hexdump(recvbuf, rlen); | ||
| /* | ||
| * Just dump strings found in the response data. | ||
| * Skip the irst 0x90 (RPC wrappers). |
| void | ||
| discon_usage(void) | ||
| { | ||
| printf(gettext("usage: smbutil discon [connection options] " |
There was a problem hiding this comment.
Won't ignoring the return value where make lint complain?
| CPPFLAGS.first += -I../common | ||
| CPPFLAGS= $(CPPFLAGS.first) |
There was a problem hiding this comment.
It looks like you're not even using any prior value of CPPFLAGS though. This seems strangely constructed.
Why not just CPPFLAGS = -I ../common $(CPPFLAGS) -- assuming you want to prepend and not just eliminate other CPPFLAGS values?
There was a problem hiding this comment.
I must have been running into some problem I've since forgotten about.
I'll have to look into that.
| CPPFLAGS += -I$(SRC)/uts/common | ||
| CPPFLAGS += -D_FAKE_KERNEL | ||
|
|
||
| C99MODE= -xc99=%all |
There was a problem hiding this comment.
Should the newer CSTD=... macro be defined here?
There was a problem hiding this comment.
Thanks. This may be why I was getting compiler errors with __unused above.
| CPPFLAGS += -I$(SRC)/uts/common | ||
| CPPFLAGS += -D_FAKE_KERNEL | ||
|
|
||
| C99MODE= -xc99=%all |
There was a problem hiding this comment.
Same CSTD comment here.
| int offset; | ||
|
|
||
| mblk_cache = kmem_cache_create("streams_mblk", sizeof (mblk_t), 32, | ||
| NULL, NULL, NULL, NULL, NULL, mblk_kmem_flags); |
There was a problem hiding this comment.
The more I think about this, the more I wonder why we need to bother with the whole separate caches for kernel space mblks. Is there value in keeping this complexity for your emulation layer?
There was a problem hiding this comment.
Was just doing what got me there in the shortest amount of time. :)
Is there anything else here you think I should chop out?
I got rid of everything that was easy to take out.
|
|
||
| /*ARGSUSED*/ | ||
| mblk_t * | ||
| esballoca(unsigned char *base, size_t size, uint_t pri, frtn_t *frp) |
There was a problem hiding this comment.
I'd be fairly stunned if any of your user space code or even your in-kernel code ever neeed desballoc or esballoca. These are for device drivers that want to loan up premapped buffers, and should not be called by anything that isn't a hardware driver I believe.
| } | ||
|
|
||
| #ifdef _KERNEL | ||
|
|
There was a problem hiding this comment.
Another chunk of code we don't need.
| * removed once a copy-callback routine is made available. | ||
| */ | ||
| if (dp->db_type == M_MULTIDATA) { | ||
| #ifdef _KERNEL |
There was a problem hiding this comment.
Kill it with fire. In fact I'd just nuke this whole block... M_MULTIDATA will never ever be seen in your user space code. It only happens for certain legacy SPARC drivers. (Cassini.)
| # Use is subject to license terms. | ||
| # | ||
| # Copyright 2017 Nexenta Systems, Inc. All rights reserved. | ||
| # |
There was a problem hiding this comment.
Are you linting this stuff?!?!?! For the love of God, why???? I mean, lint the kernel bits sure. But this is not deployed to prod, and modern GCC gives pretty decent warnings. I'd just skip lint on the user framework altogether rather than run it and suppress all these warnings...
| kidmap_get_destroy(idmap_get_handle_t *get_handle); | ||
|
|
||
| #ifdef _KERNEL | ||
| /* |
There was a problem hiding this comment.
Same case as other copied code -- was just trying to keep these like the originals.
(for now, anyway)
| * fid_pad will force the alignment. | ||
| */ | ||
| #define MAXFIDSZ 64 | ||
| #define OLD_MAXFIDSZ 16 |
There was a problem hiding this comment.
I find that with headers, it's a bit more time consuming to figure out what can be removed.
You OK with leaving these (mostly) the same as the originals?
| extern avl_tree_t vskstat_tree; | ||
| extern kmutex_t vskstat_tree_lock; | ||
|
|
||
| #if defined(_KERNEL) || defined(_FAKE_KERNEL) |
There was a problem hiding this comment.
Aren't we always _FAKE_KERNEL? Or am I missing something?
There was a problem hiding this comment.
In these too, I found it useful to keep all these headers as close as possible to the originals.
| # Common Development and Distribution License (the "License"). | ||
| # You may not use this file except in compliance with the License. | ||
| # | ||
| # You can obtain a copy of the license at usr/src/OPENSOLARIS.LICENSE |
There was a problem hiding this comment.
I'm somewhat stunned you bothered to put in Makefiles for SPARC ... has anyone actually tried building this on SPARC?
| SMB client but is very useful for some kinds of development work. | ||
|
|
||
| The architecture of this roughly parallels the in-kernel version, | ||
| where the kernel modules are build as libraries including: |
gwr
force-pushed
the
smb2clnt1f
branch
3 times, most recently
from
134157b
to
bdac70a
Compare
NEX-16818 Add fksmbcl development tool Reviewed by: Evan Layton <evan.layton@nexenta.com> Reviewed by: Matt Barden <matt.barden@nexenta.com> and: (fix ref leaks, lint) Fix libsmbfs build smbfs_ntacl.c error at __unused Catch up with later libfakekernel stuff smbfs_ntacl.c review by @sjorge Chop out some stuff in fake_stream.c Don't bother to lint under $SRC/lib/smbclnt Consider small-content Makefiles as new files fix .dbxrc, nits, add to README Catch up with C99MODE/CSTD Go back to inline in smbfs_ntacl.c now CSTD is fixed. Kill lgrep in libsmbfs Fix fksmbcl_main.c lint nit
NEX-16824 SMB client connection setup rework NEX-15557 SMB client tries to connect twice Reviewed by: Evan Layton <evan.layton@nexenta.com> Reviewed by: Matt Barden <matt.barden@nexenta.com> and session key changes
NEX-14666 Need to provide SMB 2.1 Client NEX-17187 panic in smbfs_acl_store NEX-17231 smbfs create xattr files finds wrong file NEX-17232 SMB client reconnect failures NEX-17224 smbfs lookup EINVAL should be ENOENT NEX-17260 SMB1 client fails to list directory after NEX-14666 NEX-17264 SMB client test tp_smbutil_013 fails after NEX-14666 Reviewed by: Evan Layton <evan.layton@nexenta.com> Reviewed by: Matt Barden <matt.barden@nexenta.com> Reviewed by: Rick McNeal <rick.mcneal@nexenta.com> Reviewed by: Saso Kiselkov <saso.kiselkov@nexenta.com> Reviewed by: Joyce McIntosh <joyce.mcintosh@nexenta.com> and: (cleanup, fix missing include, improve debug)
|
Updated now that 9873 has landed. We're down to 3 commits (Yea!:) |
Reviewed by: Gordon Ross <gordon.ross@nexenta.com> Reviewed by: Joyce McIntosh <joyce.mcintosh@nexenta.com>
|
Still waiting for reviewers... |
|
You may need to prod folks on IRC and/or the mailing list. I'm not sure how many people actually get the GitHub notifications for things they're not already participating in! |
|
FYI, we have merged this code (there may have been mods, I will follow up) into our code base, and are using it in our shipping product at this point. |
|
It seems that the three remaining commits have had some amount of qualified internal review at Nexenta, and as Garrett mentions the changes are also used and shipped in at least one other distribution. I think it would be fine to signal to the developer list that you're intending to integrate, then let a couple of days go past (presumably without comment) and then just proceed to RTI with notes about the testing/shipping. |
Hear hear. I've had this in an OmniOS branch for a while too but do not have an AD environment for testing the integration there. I've read over the code and didn't spot anything but unfortunately don't know this area (or the protocol) well enough to be able to review properly. |
|
Pushed to illumos/master adee678 |
Getting this ready to integrate after several cherry-pick and squash reorganizations from
https://github.com/Nexenta/illumos-nexenta/commits/release-4.0.5-FP
Yes, all of this is running in production.
BTW, I know illumos does not use pull requests, and this is not a request to "merge".
This is just an easy way to get a relatively large stack of changes up for review.
Update: I've learned that reorganizing and rebase folowed by "push -f" to the branch used by a PR like this works OK, so I've been doing that as each chunk of this work lands upstream.
Resolved comments typically show up with an "outdated" diff.
Good to know github PRs work ok with the branch changing that way.