Re: CS_SERVERADDR tag is not handled correctly in ct_con_props

Frediano,

Thanks for looking into this.  I also find the code a bit confusing.  My
answers to your comments are interspersed below.

Steve


On Thu, Jan 9, 2014 at 7:03 PM, Frediano Ziglio <[email protected]> wrote:

> 2014/1/6 Stephen Marshall <[email protected]>:
> > On Mon, Jan 6, 2014 at 12:21 PM, Frediano Ziglio <[email protected]>
> wrote:
> >
> >> 2014/1/6 Stephen Marshall <[email protected]>:
> >> > Let me try to explain in better detail:
> >> >
> >> > Problem:
> >> > The CS_SERVERADDR enum is not properly honored in the ct_con_props
> >> function
> >> > call.
> >> > For example, this call should effectively set the host and port for a
> DB
> >> > connection, but it results in an error when the connection
> information is
> >> > actually used.
> >> >     ct_con_props( g_connection,
> >> >                       CS_SET,
> >> >                       CS_SERVERADDR,
> >> >                       (CS_VOID*)server,
> >> >                       CS_NULLTERM,
> >> >                       (CS_INT*)NULL
> >> >                     );
> >> > Here: g_connection is a pointer to type CS_CONNECTION, and server is
> text
> >> > of the form "<host> <port>".  Note that FreeTDS provides other ways to
> >> > specify host and port (e.g. setting the server parameter to
> >> > "<host>:<port>"; however, the method using CS_SERVERADDR does not
> work.
> >> >
> >>
> >> Now I got it.
> >>
> >> > Approaches to fix:
> >> > I see two ways to attack this problem.  Of course, you may see others.
> >> >
> >> > One approach is to ensure port information is set in the CS_CONNECTION
> >> data
> >> > structures and then passed to the other internal data structures used
> to
> >> > establish a connection in FreeTDS.  This would be a true fix to the
> >> > problem, but would require some changes to at least a few internal
> data
> >> > structures.
> >> >
> >> > Another approach is the mark the CS_SERVERADDR tag as disabled in
> >> FreeTDS.
> >> >  This would not actually fix the problem, but would allow client
> >> > applications to work around the limitation.  However, I think this
> can be
> >> > achieved by simply undefining CS_SERVERADDR in cspublic.h.
> >> >
> >> > If you have any thoughts on ways to fix this, I'm certainly open to
> >> > suggestions.
> >> >
> >> > Steve
> >> >
> >>
> >> Well, as Sybase write specification while we should stick to it we
> >> should just handle properly this tag. Perhaps the problem is how to
> >> fit this with other port specification. Should CS_SERVERADDR port
> >> override CS_SERVERNAME "host:port" FreeTDS syntax? Perhaps we should
> >> just document the combination as not supported or undefined behaviour.
> >>
> >
> >
> > You raise an interesting idea to use CS_SERVERADDR to set the
> > CS_SERVERNAME.   This seems easier than either of my suggestions.  The
> main
> > complexity is that servername is passed directly to the ct_connect
> > function, while CS_SERVERADDR is set ahead of time in the CS_CONNECTION
> > data structure.
> >
> > I have implemented your suggestion in the attached patch file for
> > src/ctlib/ct.c.   This required small changes to both ct_con_props and
> > ct_connect.  I tested this change using sqsh 2.4, but creating a test
> > within FreeTDS itself would be a good idea.
> >
> > Let me know if this looks ok to you.   The part where I changed how
> > not-NULL values of con->server_addr are interpreted in ct_connect is the
> > only change that may be prone to side effects.  The previous logic
> appears
> > to have ignored any server_addr setting that was previously made.  Now
> > server_addr settings override the server name passed into ct_connect.
> >
> >
> >
> Hi,
>   I was looking at your patch and at Gerhard one. They really touch
> same piece of code fixing (it seems) different things. The patch from
> Gerhard just add name resolution if a CS_SERVERADDR is passed while
> your avoid to parse configuration name (or perhaps do you have same
> problem raised by Gerhard?). Looking at the code... well... is not
> easy to understand what's is doing! Some points:
> - Sybase server name can be different from dns name but your patch
> just use dns name so we'll have problems with Sybase servers;
>
[Steve]  - The CS_SERVERADDR is suppose to use host and port, not Sybase
server name, so I think this is appropriate.

- name resolution should be done so Gerhard patch is correct (I don't
> understand why name resolution is not done in a single place);
>
[Steve] - I thought hostname resolution (dns name to ip address) was done
at a lower level.

> - current code is actually wrong as it ignores servername passed to
> ct_connect if CS_SERVERADDR is used. None of the two patches fix this
> issue;
>
[Steve] - Since server name is primarily used to lookup host and port, you
really only have two choices if both are specified:
1. Prefer server name passed to ct_connect of CS_SERVERADDR
2. Prefer CS_SERVERADDR over server name

The code previoulsy did #1, my patch changed it to do #2.  The rationale
for this was based on behavior in sqsh using both FreeTDS and Sybase
ctlib.  When the -S <host>:<port> syntax is used, sqsh passes host and port
as a CS_SERVERADDR parameter AND it passes the <host>:<port> string as the
servername to ct_connect.  When sqsh is linked against ctlib, the
CS_SERVERADDR values were honored, indicating ctlib used method #2.
Therefore, I think changing FreeTDS to use method #2 would be a good idea.


- is not clear what the difference setting server to NULL or "";
>
[Steve] - I don't know either.

> - should configuration NEVER be read if CS_SERVERADDR is used?

[Steve] - The only additional issue to what I discussed above is the case
where you could look up parameters other than host and port by server name,
e.g. tds version or client charset.  However, I think that if someone is
explicitly passing host and port numbers, it is unlikely they are relying
on the configuration file for anything, so I don't think this would be bad
behavior.  In other words, if CS_SERVERADDR is used, client should be
expect to ct_con_props to set other pieces of configuration, like protocol
version, client character encoding.

Looking at libTDS code (tds_read_config_info) seems that an attempt to read
> configuration is always done, even for server:port syntax. However
> setting server_name to "" should cause only global section to be
> parsed.
>
> Well.. just trying to collect some details... no solution yet.
> Possibly Gerhard patch adding set to server_name after reading
> configuration if servername is valid and CS_SERVERADDR is used (in the
> "if (con->server_addr)" statement) ?
>
> Frediano
> _______________________________________________
> FreeTDS mailing list
> [email protected]
> http://lists.ibiblio.org/mailman/listinfo/freetds
>