[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v5 3/4] net/net: Convert parse_host_port() to Er
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH v5 3/4] net/net: Convert parse_host_port() to Error |
Date: |
Wed, 28 Jun 2017 07:51:54 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/25.2 (gnu/linux) |
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.
[Qemu-devel] [PATCH v5 2/4] net/socket: Convert error message to Error, Mao Zhongyi, 2017/06/26
[Qemu-devel] [PATCH v5 4/4] net/socket: Improve -net socket error reporting, Mao Zhongyi, 2017/06/26