Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Allow . as hostname in pipe names#17284

Merged
saurabh500 merged 3 commits into
dotnet:masterfrom
saurabh500:fixDotAsHost
Mar 21, 2017
Merged

Allow . as hostname in pipe names#17284
saurabh500 merged 3 commits into
dotnet:masterfrom
saurabh500:fixDotAsHost

Conversation

@saurabh500
Copy link
Copy Markdown
Contributor

@saurabh500 saurabh500 commented Mar 19, 2017

Fixes https://github.com/dotnet/corefx/issues/17282

Since Uri cannot handle . as hostname, the changes include splitting the named pipe data source by \ and generating the hostname and pipename from the split parts.

Apart from the tests added, I ran some tests with a SqlServer to see if the connectivity works.

cc @corivera @geleems

@saurabh500 saurabh500 requested review from AArnott, corivera and geleems and removed request for AArnott March 19, 2017 19:18
@saurabh500 saurabh500 self-assigned this Mar 19, 2017
@saurabh500 saurabh500 added this to the 2.0.0 milestone Mar 20, 2017
geleems
geleems reviewed Mar 20, 2017
View reviewed changes
Comment thread src/System.Data.SqlClient/src/System/Data/SqlClient/SNI/SNIProxy.cs
}

string[] absolutePathParts = uri.AbsolutePath.Split(ForwardSlashSeparator);
string host = tokensByBackSlash[2];
Copy link
Copy Markdown

@geleems geleems Mar 20, 2017

Choose a reason for hiding this comment

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

Can we do string host = tokensByBackSlash[2].Trim();?

Copy link
Copy Markdown
Contributor Author

@saurabh500 saurabh500 Mar 20, 2017

Choose a reason for hiding this comment

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

No. Np:\.\pipe\sql\query would lead to a successful connection but np:\. \pipe\sql\query doesn't lead to a successful connection in Native SNI.
We shouldnt trim the pipe parts.

Comment thread src/System.Data.SqlClient/src/System/Data/SqlClient/SNI/SNIProxy.cs
// Another valid Sql named pipe for an named instance is \\.\pipe\MSSQL$MYINSTANCE\sql\query
if (absolutePathParts.Length < 4)
//Check if the "pipe" keyword is the first part of path
if (!PipeToken.Equals(tokensByBackSlash[3]))
Copy link
Copy Markdown

@geleems geleems Mar 20, 2017

Choose a reason for hiding this comment

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

also if (!PipeToken.Equals(tokensByBackSlash[3].Trim()))?

Copy link
Copy Markdown
Contributor Author

@saurabh500 saurabh500 Mar 20, 2017

Choose a reason for hiding this comment

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

No trimming of pipe paths.

Comment thread src/System.Data.SqlClient/src/System/Data/SqlClient/SNI/SNIProxy.cs

for ( int i = 4; i < tokensByBackSlash.Length-1; i++)
{
pipeNameBuilder.Append(tokensByBackSlash[i]);
Copy link
Copy Markdown

@geleems geleems Mar 20, 2017

Choose a reason for hiding this comment

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

Can we do pipeNameBuilder.Append(tokensByBackSlash[i].Trim()); ?

Copy link
Copy Markdown
Contributor Author

@saurabh500 saurabh500 Mar 20, 2017

Choose a reason for hiding this comment

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

Same as above

Comment thread src/System.Data.SqlClient/src/System/Data/SqlClient/SNI/SNIProxy.cs
pipeNameBuilder.Append(Path.PathSeparator);
}
// Append the last part without a "/"
pipeNameBuilder.Append(tokensByBackSlash[tokensByBackSlash.Length - 1]);
Copy link
Copy Markdown

@geleems geleems Mar 20, 2017

Choose a reason for hiding this comment

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

What if connection string is \\.\pipe\sql\query\ ?

Copy link
Copy Markdown
Contributor Author

@saurabh500 saurabh500 Mar 20, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

SqlClient should fail to connect in this case.
With the change above a blank will be appended after the last \ and there will be a failure to connect

corivera
corivera reviewed Mar 21, 2017
View reviewed changes
Comment thread src/System.Data.SqlClient/src/System/Data/SqlClient/SNI/SNIProxy.cs
ReportSNIError(SNIProviders.NP_PROV);
return false;
}

Copy link
Copy Markdown
Member

@corivera corivera Mar 21, 2017

Choose a reason for hiding this comment

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

Check for leading backslashes?

Copy link
Copy Markdown
Member

@corivera corivera Mar 21, 2017

Choose a reason for hiding this comment

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

Disregard this, there's a check for it higher up.

@saurabh500 saurabh500 merged commit 6efbe44 into dotnet:master Mar 21, 2017
@saurabh500 saurabh500 deleted the fixDotAsHost branch May 2, 2017 22:00
dotnet-bot pushed a commit that referenced this pull request Mar 30, 2018
@ahsonkhan @dotnet-bot
Use ROSpan.IndexOf as the workhorse for string.IndexOf (#17284)
c517037 
* Use ROSpan.IndexOf as the workhorse for string.IndexOf

* Make changes to Span.IndexOf to follow what string.IndexOf did.

* Address PR feedback.

* Use Unsafe.Read instead of Unsafe.ReadUnaligned.

* Remove special casing for count == 0

* Fix up debug assert to use vector count instead of intptr.size

* Use size of Vector<ushort> instead of Vector<byte>.Count

Signed-off-by: dotnet-bot-corefx-mirror <dotnet-bot@microsoft.com>
dotnet-bot pushed a commit that referenced this pull request Mar 30, 2018
@ahsonkhan @dotnet-bot
Use ROSpan.IndexOf as the workhorse for string.IndexOf (#17284)
8c4001a 
* Use ROSpan.IndexOf as the workhorse for string.IndexOf

* Make changes to Span.IndexOf to follow what string.IndexOf did.

* Address PR feedback.

* Use Unsafe.Read instead of Unsafe.ReadUnaligned.

* Remove special casing for count == 0

* Fix up debug assert to use vector count instead of intptr.size

* Use size of Vector<ushort> instead of Vector<byte>.Count

Signed-off-by: dotnet-bot-corefx-mirror <dotnet-bot@microsoft.com>
dotnet-bot pushed a commit that referenced this pull request Mar 30, 2018
@ahsonkhan @dotnet-bot
Use ROSpan.IndexOf as the workhorse for string.IndexOf (#17284)
aa9a7c4 
* Use ROSpan.IndexOf as the workhorse for string.IndexOf

* Make changes to Span.IndexOf to follow what string.IndexOf did.

* Address PR feedback.

* Use Unsafe.Read instead of Unsafe.ReadUnaligned.

* Remove special casing for count == 0

* Fix up debug assert to use vector count instead of intptr.size

* Use size of Vector<ushort> instead of Vector<byte>.Count

Signed-off-by: dotnet-bot-corefx-mirror <dotnet-bot@microsoft.com>
dotnet-bot pushed a commit that referenced this pull request Mar 30, 2018
@ahsonkhan @dotnet-bot
Use ROSpan.IndexOf as the workhorse for string.IndexOf (#17284)
3f125e1 
* Use ROSpan.IndexOf as the workhorse for string.IndexOf

* Make changes to Span.IndexOf to follow what string.IndexOf did.

* Address PR feedback.

* Use Unsafe.Read instead of Unsafe.ReadUnaligned.

* Remove special casing for count == 0

* Fix up debug assert to use vector count instead of intptr.size

* Use size of Vector<ushort> instead of Vector<byte>.Count

Signed-off-by: dotnet-bot-corefx-mirror <dotnet-bot@microsoft.com>
ahsonkhan pushed a commit that referenced this pull request Mar 30, 2018
@dotnet-bot @ahsonkhan
Use ROSpan.IndexOf as the workhorse for string.IndexOf (#17284) (#28635)
1ad8ed9 
* Use ROSpan.IndexOf as the workhorse for string.IndexOf

* Make changes to Span.IndexOf to follow what string.IndexOf did.

* Address PR feedback.

* Use Unsafe.Read instead of Unsafe.ReadUnaligned.

* Remove special casing for count == 0

* Fix up debug assert to use vector count instead of intptr.size

* Use size of Vector<ushort> instead of Vector<byte>.Count

Signed-off-by: dotnet-bot-corefx-mirror <dotnet-bot@microsoft.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers

@corivera corivera corivera approved these changes

@geleems geleems geleems approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@saurabh500 saurabh500

Labels

area-System.Data.SqlClient

Projects

None yet

Milestone

2.0.0

Development

Successfully merging this pull request may close these issues.

4 participants

@saurabh500 @corivera @geleems @dnfclas