[Ovirt-devel] [PATCH viewer] few minor bugfixes

[Mohammed Morsi]

Daniel P. Berrange wrote:
> On Mon, Jun 01, 2009 at 04:42:08PM -0400, Mohammed Morsi wrote:
>   
>>>> -#if defined(HAVE_SOCKET) && defined(HAVE_CONNECT) && defined(HAVE_HTONS) && defined(HAVE_GETHOSTBYNAME)
>>>> +#if defined(HAVE_SOCKET) && defined(HAVE_CONNECT) && defined(HAVE_HTONS)
>>>>  
>>>>  static int 
>>>>  viewer_open_vnc_socket(const char* vnchost, int vncport)
>>>>  {
>>>>    int socketfd;
>>>> -  struct hostent *serv;
>>>>    struct sockaddr_in serv_addr;
>>>>  
>>>>    socketfd = socket(PF_INET, SOCK_STREAM, 0);
>>>> @@ -917,14 +918,9 @@ viewer_open_vnc_socket(const char* vnchost, int vncport)
>>>>        return -1;
>>>>    }
>>>>  
>>>> -  serv = gethostbyname(vnchost);
>>>> -  if(serv == NULL){
>>>> -      return -1;
>>>> -  }
>>>> -
>>>>    serv_addr.sin_family = PF_INET;
>>>>    serv_addr.sin_port = htons(vncport);
>>>> -  serv_addr.sin_addr.s_addr = ((struct in_addr *)(serv->h_addr))->s_addr; 
>>>> +  serv_addr.sin_addr.s_addr = inet_addr(vnchost);
>>>>  
>>>>    if (connect(socketfd,(struct sockaddr *)&serv_addr,sizeof(serv_addr)) < 0){
>>>>        return -1;
>>>> @@ -933,7 +929,7 @@ viewer_open_vnc_socket(const char* vnchost, int vncport)
>>>>    return socketfd;
>>>>  }
>>>>     
>>>>         
>>> This method should really be re-written to just use getaddrinfo() for
>>> lookups.
>>>
>>>       
>> Not sure what using getaddrinfo gets us here, we're just trying to
>> connect to the hostname / port that we already have. In any case what
>> would be setting the service parameter to, since the port which we are
>> connecting is the autogenerated local tunnel port.
>>     
>
> Re-factoring code which uses gethostbyname() is just shifting deck-chairs
> on the titanic. Sooner or later it will need to die and use getaddrinfo().
> Even for your mere 'localhost' case, it would work better because it
> would correctly return either IPv4 (127.0.0.1) or IPv6 (::1) or both, 
> sorted according to which protocols were enabled on the machine in question.
> It is just wasted development effort to write code which uses anything else
> these days.
>
> Daniel
>   
I'm still not fully sure why we need the call to getaddrinfo when we
already have and are using the ip address and port which to connect to.
We are using the actual '127.0.0.1' ip as opposed to the 'localhost' dns
name, so we should be safe when it comes to resolving the correct target.

Regardless, I've no objections to adding getaddrinfo to only get the
list of addrinfo's for the matching targets on the machine, but I'm
still uncertain as to what the string "service" would be, as how we can
derive that from the int "port" variable that we have (do we simple
sscanf the int into the string to pass in?)

   Appreciate the help,
      -Mo