Re: R: Re: Way to get connection params on failure?

Don't mind. This is what tests are about. The fact they can discover
defect in few time means they works as expected!

It would be even great if your code would have some test coverage. At
http://freetds.sourceforge.net/up/out92/coverage/src/dblib/dblib.c.gcov.html
you cannot see any on the lines you added.

Frediano

2014/1/19 Marc Abramowitz <[email protected]>:
> Ah ok. Oops. sorry 'bout that!
>
>
> On Sun, Jan 19, 2014 at 6:24 AM, Frediano Ziglio <[email protected]> wrote:
>>
>> Hi,
>>   I had to do a commit as your patch caused a crash as dbproc or
>> tds_socket could be null
>>
>> Frediano
>>
>> -------- Messaggio originale --------
>> Da: Marc Abramowitz <[email protected]>
>> Data: 18/01/2014  18:00  (GMT+00:00)
>> A: FreeTDS Development Group <[email protected]>
>> Cc: FreeTDS Development Group <[email protected]>
>> Oggetto: Re: [freetds] Way to get connection params on failure?
>>
>> Cool! Thanks!
>>
>> -Marc
>> http://marc-abramowitz.com
>> Sent from my iPhone 4S
>>
>>
>> On Jan 18, 2014, at 5:53 AM, Frediano Ziglio <[email protected]> wrote:
>>
>> > Committed!
>> >
>> > Frediano
>> >
>> > 2014/1/18 Marc Abramowitz <[email protected]>:
>> >> Great comments, thanks!
>> >>
>> >> I updated the merge request and also went ahead and created a squashed
>> >> version with a single commit:
>> >>
>> >> https://gitorious.org/freetds/freetds/merge_requests/26
>> >>
>> >> Let me know if you want other changes.
>> >>
>> >>
>> >> On Mon, Jan 13, 2014 at 5:22 AM, Frediano Ziglio <[email protected]>
>> >> wrote:
>> >>
>> >>> Hi,
>> >>>  patch is nicer. However is construct_message is already used you get
>> >>> a leak as you not free the buffer allocated before (your buffer is
>> >>> then freed after calling the error handler). Also are you sure you
>> >>> want to add the server name details every time you have a login? Note
>> >>> that login field is NULL if you are connected so probably you will
>> >>> have a core for errors after the connection! I would also use asprintf
>> >>> to make buffer computation easier.
>> >>>
>> >>> Something like
>> >>>
>> >>> if (dbproc->tds_socket->login &&
>> >>> !tds_dstr_isempty(&dbproc->tds_socket->login->server_name)) {
>> >>>     char * buffer = NULL;
>> >>>     if (asprintf(&buffer, "%s (%s)", msg->msgtext,
>> >>> tds_dstr_cstr(&dbproc->tds_socket->login->server_name)) >= 0) {
>> >>>         free((char*) constructed_message.msgtext);
>> >>>         constructed_message.msgtext = buffer;
>> >>>         constructed_message.severity = msg->severity;
>> >>>         msg = &constructed_message;
>> >>>    }
>> >>> }
>> >>>
>> >>> Frediano
>> >>>
>> >>> 2014/1/10 Marc Abramowitz <[email protected]>:
>> >>>> Hi Freddy,
>> >>>>
>> >>>> I just updated the merge request so that I don't access the DSTR
>> >>>> fields
>> >>>> directly.
>> >>>>
>> >>>> As for the buffer being freed, I also had concerns about that, but I
>> >>> think
>> >>>> it's okay. You will notice that this code is very similar to the code
>> >>> right
>> >>>> above it for doing the dynamic parameter substitution -- they both
>> >>>> do:
>> >>>>
>> >>>> ```
>> >>>>        constructed_message.msgtext = buffer;
>> >>>> ```
>> >>>>
>> >>>> Then later on this happens:
>> >>>>
>> >>>> ```
>> >>>>  /* we're done with the dynamic string now. */
>> >>>>  free((char*) constructed_message.msgtext);
>> >>>> ```
>> >>>>
>> >>>> so that should free the buffer. So I think it's good.
>> >>>>
>> >>>> I have not looked at libtds. pymssql only uses dblib, so I don't have
>> >>>> a
>> >>> use
>> >>>> case and it would be harder for me to test. Perhaps someone else
>> >>>> would be
>> >>>> better suited for moving it to libtds?
>> >>>>
>> >>>> Let me know if other changes are needed or if you want the commits
>> >>>> squashed, etc.
>> >>>>
>> >>>> Cheers,
>> >>>> Marc
>> >>>>
>> >>>>
>> >>>> On Thu, Jan 9, 2014 at 3:19 AM, Frediano Ziglio <[email protected]>
>> >>> wrote:
>> >>>>
>> >>>>> Hi,
>> >>>>>  Dstr fields should not be acccessed directly. Are you sure buffer
>> >>>>> is
>> >>>>> freed?
>> >>>>>
>> >>>>> Beside this patch looks good. Perhaps should be moved in libtds so
>> >>>>> all
>> >>>>> layers will use this code.
>> >>>>>
>> >>>>> About dynamic parameters values should be controlled as are only
>> >>>>> errors
>> >>>>> from our library, not from server
>> >>>>>
>> >>>>> Frediano
>> >>>>> Il 06/gen/2014 19:08 "Marc Abramowitz" <[email protected]> ha
>> >>>>> scritto:
>> >>>>>
>> >>>>>> On Sun, Jan 5, 2014 at 11:32 AM, Frediano Ziglio
>> >>>>>> <[email protected]>
>> >>>>>> wrote:
>> >>>>>>
>> >>>>>>> The problem of your implementation is that TDSECONN is passed by
>> >>>>>>> libTDS and not all libraries add the required parameter so you can
>> >>>>>>> have a not formatted string in other libraries.
>> >>>>>>>
>> >>>>>>> Mixing normal string and string with format looks a bit security
>> >>>>> suspect
>> >>>>>>> to me.
>> >>>>>>
>> >>>>>> Yeah, that smelled a little funny to me too.
>> >>>>>>
>> >>>>>> I updated my PR so that dbperror itself appends the server_name, if
>> >>>>>> available, to the error message. So no need to pass it into
>> >>>>>> dbperror.
>> >>>>>>
>> >>>>>> https://gitorious.org/freetds/freetds/merge_requests/24/diffs
>> >>>>>>
>> >>>>>> Hopefully, that's better.
>> >>>>>>
>> >>>>>> Marc
>> >>>>>>
>> _______________________________________________
>> FreeTDS mailing list
>> [email protected]
>> http://lists.ibiblio.org/mailman/listinfo/freetds
>
>