Re: [Qemu-devel] [PATCH v5 3/4] net/net: Convert parse_host_port() to Error

[Markus Armbruster]

Mao Zhongyi <address@hidden> writes:

> Hi, Markus
>
> On 06/27/2017 04:04 PM, Markus Armbruster wrote:
>> Mao Zhongyi <address@hidden> writes:
>>
>>> Cc: address@hidden
>>> Cc: address@hidden
>>> Cc: address@hidden
>>> Cc: address@hidden
>>> Cc: address@hidden
>>> Signed-off-by: Mao Zhongyi <address@hidden>
>>> ---
>>>  include/qemu/sockets.h |  3 ++-
>>>  net/net.c              | 22 +++++++++++++++++-----
>>>  net/socket.c           | 19 ++++++++++++++-----
>>>  3 files changed, 33 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/include/qemu/sockets.h b/include/qemu/sockets.h
>>> index 5c326db..78e2b30 100644
>>> --- a/include/qemu/sockets.h
>>> +++ b/include/qemu/sockets.h
>>> @@ -53,7 +53,8 @@ void socket_listen_cleanup(int fd, Error **errp);
>>>  int socket_dgram(SocketAddress *remote, SocketAddress *local, Error 
>>> **errp);
>>>
>>>  /* Old, ipv4 only bits.  Don't use for new code. */
>>> -int parse_host_port(struct sockaddr_in *saddr, const char *str);
>>> +int parse_host_port(struct sockaddr_in *saddr, const char *str,
>>> +                    Error **errp);
>>>  int socket_init(void);
>>>
>>>  /**
>>> diff --git a/net/net.c b/net/net.c
>>> index 6235aab..884e3ac 100644
>>> --- a/net/net.c
>>> +++ b/net/net.c
>>> @@ -100,7 +100,8 @@ static int get_str_sep(char *buf, int buf_size, const 
>>> char **pp, int sep)
>>>      return 0;
>>>  }
>>>
>>> -int parse_host_port(struct sockaddr_in *saddr, const char *str)
>>> +int parse_host_port(struct sockaddr_in *saddr, const char *str,
>>> +                    Error **errp)
>>>  {
>>>      char buf[512];
>>>      struct hostent *he;
>>> @@ -108,24 +109,35 @@ int parse_host_port(struct sockaddr_in *saddr, const 
>>> char *str)
>>>      int port;
>>>
>>>      p = str;
>>> -    if (get_str_sep(buf, sizeof(buf), &p, ':') < 0)
>>> +    if (get_str_sep(buf, sizeof(buf), &p, ':') < 0) {
>>> +        error_setg(errp, "The address should contain ':', for example: "
>>> +                   "xxxx=230.0.0.1:1234");
>>
>> Suggest "Host address '%s' should ..." like you do in the next error message.
>>
>> The xxxx= is confusing.  Do we need an example here?  The other error
>> messages in this function apparently don't.
>>
>> What about "host address '%s' doesn't contain ':' separating host from
>> port" or "can't find ':' separating host from port in host address
>> '%s'"?
>>
>
> Yes, my description message is really confusing.
> Both of your prompt message are well understood.
>
> Thank you very much.
>
>>
>>>          return -1;
>>> +    }
>>>      saddr->sin_family = AF_INET;
>>>      if (buf[0] == '\0') {
>>>          saddr->sin_addr.s_addr = 0;
>>>      } else {
>>>          if (qemu_isdigit(buf[0])) {
>>> -            if (!inet_aton(buf, &saddr->sin_addr))
>>> +            if (!inet_aton(buf, &saddr->sin_addr)) {
>>> +                error_setg(errp, "Host address '%s' is not a valid "
>>> +                           "IPv4 address", buf);
>>>                  return -1;
>>> +            }
>>>          } else {
>>> -            if ((he = gethostbyname(buf)) == NULL)
>>> +            he = gethostbyname(buf);
>>> +            if (he == NULL) {
>>> +                error_setg(errp, "Specified hostname is error.");
>>
>> No.  Suggest "can't resolve host address '%s'.  This error message still
>> lacks detail, but I'm not sure hstrerror() is sufficiently portable.
>>
>
> The gethostbyname() return a null pointer if an error occurs, and the h_errno
> variable holds an error number. herror() and hstrerror() can prints the error
> message associated with the current value of h_errno, but hstrerror() returns
> the string type is good for passing the error message to Error. So I'd prefer
> the hstrerror.
>
> As for the portability of hstrerror(), sorry, I'm also not sure, but in this
> case I tested, it's OK. so I want to use hstrerror() for a while, if there are
> any problem that can be fixed later. Do you think it can be done?

Standard first portability question: does Windows provide it?

>> Outside this patch's scope: gethostbyname() is obsolete.  Applications
>> should use getaddrinfo() instead.  Comes with gai_strerror().
>
> Can I try to fix it later?

Sure.  By "outside this patch's scope" I mean it's a separate matter
that clearly doesn't have to be addressed to get this patch accepted.