MIME-Version: 1.0
In-Reply-To: <CANda38rGXpq8EuFnok6_NuRGTfdk1RQAwJzSYufom4OhhMBsag@mail.gmail.com>
X-Mailer: MIME-tools 5.505 (Entity 5.505)
Content-Disposition: inline
X-RT-Interface: Web
References: <CANda38rGXpq8EuFnok6_NuRGTfdk1RQAwJzSYufom4OhhMBsag@mail.gmail.com>
 <RT-Ticket-42834@isc.org>
Content-Type: text/plain; charset="utf-8"
Message-ID: <rt-4.2.8-25287-1472189692-832.42834-0-0@isc.org>
Content-Transfer-Encoding: binary
X-RT-Original-Encoding: utf-8
RT-Send-CC:
X-RT-Encrypt: 0
X-RT-Sign: 0
Content-Length: 2153

On Tue Jul 12 12:39:52 2016, pinjalaganeshb@gmail.com wrote:
> Hi,
> 
> This is Ganesh P , Would like to know whether the look-up logic of
> omapi_io_states is appropriate when picking up the correct state
> information  for the given ( input ) cbarg.
> 
> Existing Logic:
> 
> omapi_iscsock_cb(isc_task_t   *task,
>                  isc_socket_t *socket,
>                  void         *cbarg,
>                  int           flags)
> {
> #if SOCKDELETE
>         /*
>          * walk through the io states list, if our object is on there
>          * service it.  if not ignore it.
>          */
>         for (obj = omapi_io_states.next; (obj != NULL) ; obj = obj->next) {

I think there may have been a cut and paste error.  In my copy of the 
code the check statement is (obj != NULL) && (obj->next != NULL);

>                 if (obj == cbarg) {
>                         break;
>                 }
>         }
> 
>         if (obj == NULL)  {
>                 return(0);
>         }
> #else
> }
> 
> "obj = obj->next" is this condition to be removed from for loop ? because
> of having it we might never do a comparison of last state of
> "omapi_io_states" , isn't it ?

I think you meant to refer to the (obj->next != NULL) condition rather
than the obj = obj->next statement.  It does appear to be incorrect.
I'll have to review the code more deeply to see if there is any reason
for it to be there.

> 
>  in existing code flow  ,  failing to find an appropriate
>  "omapi_io_states" ( i.e no match with "cbarg" )  can lead to invocation of
>  "status = obj->reader(obj->inner);" and hence "dhcp" process might wait on
> an UNSET FD and blocks forever ?

I think this turns out to be only possibly in theory.  I think that
in reality the matching socket is always on the list.  We either
find it by comparison with cbarg, or it is the last one on the list
and we default to using it.

That is, there appears to be a bug in the code but as the rest
of the code is generally well behaved this bug probably doesn't
cause an issue.

Have you seen any problems in this area?
> 
> Can you please clarify ?
> 
> Thanks in advance ,
> 
> Ganesh.