Re: [reSIProcate] Questions about AresDns::internalInit() method

[Scott Godin <sgodin@xxxxxxxxxxxxxxx>]

Hi Dario,

I think you are right.  Calling checkDnsChange is not supposed to modify the current operation of anything - it is only a check.  I agree we need the first set of changes you mentioned to the setting of timeout and tries.  

Looking closer at the #ifndef USE_CARES block you pointed out - I think there are bigger problems.  Looks like everytime checkDnsChange is called the mPollItems array will grow by nServers.  That is not good.  I'm thinking this block of code needs to move to AresDns::init method instead.  It also needs to be smarter to handle init being called mulitple times.  I'm pretty sure the intention of the developer that added checkDnsChange was that init should be called again it it returns true - but I'm not 100% sure.  I am making that assumption for now.

I have checked in a fix for this that Dario has already tested out.

Best Regards,

Scott Godin

On Fri, Jan 2, 2015 at 9:44 AM, Dario Bozzali <Dario.Bozzali@xxxxxxxxxxx> wrote:

Hi all,

I was looking at the file rutil/dns/AresDns.cxx, in particular at AresDns::checkDnsChange() and AresDns::internalInit() methods, and I noticed some things that I don’t understand.

 

Method internalInit() is used in init() and checkDnsChange(), but with different parameters. For example, in init() method for timeout and tries variables are used the values that have been set using DnsStub::setDnsTimeoutAndTries(), but in checkDnsChange() method it is used the default value (0) and in internalInit() method mChannel data member is changed instead of (I suppose) the local variable *channel (see lines 374-382).

      if (timeout > 0)

      {

         mChannel->timeout = timeout;

      }

 

      if (tries > 0)

      {

         mChannel->tries = tries;

      }

I suppose it should be:

      if (timeout > 0)

      {

         (*channel)->timeout = timeout;

      }

 

      if (tries > 0)

      {

         (*channel)->tries = tries;

      }

 

Moreover I have some doubts about the following lines of code:

#ifndef USE_CARES

      if ( mPollGrp )

      {

         // expand vector to hold {nservers} and init to NULL

         mPollItems.insert( mPollItems.end(), (*channel)->nservers, (AresDnsPollItem*)0);

         // tell ares to let us know when things change

         ares_process_set_poll_cb(mChannel, AresDnsPollItem::socket_poll_cb, this);

      }

#endif

 

In a similar way as exposed before, I don’t undertand why *channel is used to change mChannel since if internalInit() is called inside checkDnsChange() then *channel is not equal to mChannel as it happens when internalInit() is called inside init().

I think that if internalInit() is used by checkDnsChange() the previous code could (or should) not be called.

 

What is your opinions about my considerations?

 

Thank you and kind regards,

Dario.

_______________________________________________
resiprocate-devel mailing list
resiprocate-devel@xxxxxxxxxxxxxxx
https://list.resiprocate.org/mailman/listinfo/resiprocate-devel