Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

9877 Want t_koptmgmt in kTLI, and 9872 #38

Closed
wants to merge 3 commits into from
Closed

Conversation

gwr
Copy link
Member

@gwr gwr commented Oct 8, 2018

Would like these integrated ahead of the rest of #37 so people can test that by just loading a new package.

mbarden and others added 2 commits October 8, 2018 00:02
NEX-8707 smb/server in 4.0.x does not accept username@hostname
Reviewed by: Evan Layton <evan.layton@nexenta.com>
Reviewed by: Gordon Ross <gordon.ross@nexenta.com>

and: (JBK review)
Part of the following change:
NEX-16824 SMB client connection setup rework
Reviewed by: Evan Layton <evan.layton@nexenta.com>
Reviewed by: Matt Barden <matt.barden@nexenta.com>
@gwr gwr self-assigned this Oct 8, 2018
buf = strdup(tmp_user.lg_e_username);
if (buf == NULL)
goto errout;
p = buf + (p - tmp_user.lg_e_username);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we need a strtok() in the kernel. I'm assuming we don't have one already.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can live with it as it is.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh wait, this is user space. Maybe just use strtok_r() instead of baking your own.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think strtok is the 'right tool' here. We already know, from strchr(), where in the string the character we want to overwrite is; Converting just the if-block to use strtok would just make us search the string for it again, and I don't see why we'd want to do that. And since we need to dup the string if we're going to modify it, we can't use strtok() to find the token in the first instance. I'd rather keep it as-is.

@@ -193,7 +201,18 @@ smbd_user_auth_logon(smb_logon_t *user_info)
token->tkn_audit_sid = entry->sa_audit_sid;
}

if (buf != NULL)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test is unnecessary -- free(NULL) is safe in user space.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Btw, it would be cool if adt_end_session() had the same safety (nop if argument is NULL). Maybe it already does? I haven't looked.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, adt_end_session is OK to call with NULL.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated as suggested.

ctlsize = sizeof (*opt_req) + optlen;
ctlbuf = kmem_alloc(ctlsize, KM_SLEEP);

/* LINTED E_BAD_PTR_CAST_ALIGN */
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually prefer to cast through (void *) for this and avoid the LINTED complaints. That tends to satisfy other compiler warning systems too. But I'm also ok with this for now.

Copy link
Member Author

@gwr gwr Oct 8, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just copied from libnsl... (or somewhere, but I think that was it:)

@gdamore
Copy link
Member

gdamore commented Oct 8, 2018 via email

@gwr
Copy link
Member Author

gwr commented Oct 10, 2018

Anyone else? I'd like to RTI this.

@gdamore
Copy link
Member

gdamore commented Oct 10, 2018

I think you should RTI it.

@gwr gwr closed this Oct 11, 2018
@gwr gwr deleted the il-ktli branch October 16, 2018 15:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
4 participants