Mao Zhongyi <maozy.f...@cn.fujitsu.com> writes: > Hi, Markus > > On 06/27/2017 04:04 PM, Markus Armbruster wrote: >> Mao Zhongyi <maozy.f...@cn.fujitsu.com> writes: >> >>> Cc: berra...@redhat.com >>> Cc: kra...@redhat.com >>> Cc: pbonz...@redhat.com >>> Cc: jasow...@redhat.com >>> Cc: arm...@redhat.com >>> Signed-off-by: Mao Zhongyi <maozy.f...@cn.fujitsu.com> >>> --- >>> 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.