On 2017年05月03日 16:59, Mao Zhongyi wrote:
On 05/03/2017 04:54 PM, Markus Armbruster wrote:
Mao Zhongyi <maozy.f...@cn.fujitsu.com> writes:
Hi, Markus,Daniel
On 04/28/2017 04:02 PM, Markus Armbruster wrote:
"Daniel P. Berrange" <berra...@redhat.com> writes:
On Thu, Apr 27, 2017 at 06:24:17PM +0200, Markus Armbruster wrote:
No review, just an observation.
Mao Zhongyi <maozy.f...@cn.fujitsu.com> writes:
Currently, net_socket_mcast_create(), net_socket_fd_init_dgram()
and
net_socket_fd_init() use the function such as fprintf(),
perror() to
report an error message.
Now, convert these functions to Error.
CC: jasow...@redhat.com, arm...@redhat.com
Signed-off-by: Mao Zhongyi <maozy.f...@cn.fujitsu.com>
---
net/socket.c | 66
+++++++++++++++++++++++++++++++++++++-----------------------
1 file changed, 41 insertions(+), 25 deletions(-)
diff --git a/net/socket.c b/net/socket.c
index b0decbe..559e09a 100644
--- a/net/socket.c
+++ b/net/socket.c
[...]
@@ -433,25 +437,27 @@ static NetSocketState
*net_socket_fd_init_stream(NetClientState *peer,
static NetSocketState *net_socket_fd_init(NetClientState *peer,
const char *model,
const char *name,
- int fd, int
is_connected)
+ int fd, int
is_connected,
+ Error **errp)
{
int so_type = -1, optlen=sizeof(so_type);
if(getsockopt(fd, SOL_SOCKET, SO_TYPE, (char *)&so_type,
(socklen_t *)&optlen)< 0) {
- fprintf(stderr, "qemu: error: getsockopt(SO_TYPE) for
fd=%d failed\n",
+ error_setg(errp, "qemu: error: getsockopt(SO_TYPE) for
fd=%d failed",
fd);
closesocket(fd);
return NULL;
}
switch(so_type) {
case SOCK_DGRAM:
- return net_socket_fd_init_dgram(peer, model, name, fd,
is_connected);
+ return net_socket_fd_init_dgram(peer, model, name, fd,
is_connected, errp);
case SOCK_STREAM:
return net_socket_fd_init_stream(peer, model, name, fd,
is_connected);
default:
/* who knows ... this could be a eg. a pty, do warn and
continue as stream */
- fprintf(stderr, "qemu: warning: socket type=%d for
fd=%d is not SOCK_DGRAM or SOCK_STREAM\n", so_type, fd);
+ error_setg(errp, "qemu: warning: socket type=%d for
fd=%d is not SOCK_DGRAM"
+ " or SOCK_STREAM", so_type, fd);
Not this patches problem: this case is odd, and the comment is
bogus.
If @fd really was a pty, getsockopt() would fail with ENOTSOCK,
wouldn't
it? If @fd is a socket, but neither SOCK_DGRAM nor SOCK_STREAM (say
SOCK_RAW), why is it safe to continue as if it was SOCK_STREAM?
Jason?
IMHO it is a problem with this patch. Previously we merely printed
a warning & carried on, which is conceptually ok in general, though
dubious here for the reason you say.
Now we are filling an Error **errp object, and carrying on - this is
conceptually broken anywhere. If an Error ** is filled, we must
return.
If we want to carry on, we shouldn't fill Error **.
You're right.
return net_socket_fd_init_stream(peer, model, name, fd,
is_connected);
IMHO, we just kill this and put return NULL here. If there is a
genuine
reason to support something like SOCK_RAW, it should be explicitly
handled, because this default: case will certainly break
SOCK_SEQPACKET
and SOCK_RDM which can't be treated as streams.
It's either magic or misguided defensive programming. Probably the
latter, but I'd like to hear Jason's opinion.
If it's *necessary* magic, we can't use error_setg(). Else, we should
drop the default, and insert a closesocket(fd) before the final return
NULL.
As Daniel said, although the previous printed warning message is
dubious, but it is conceptually ok in general. Simply filling it to
Error ** is problematic. Of course, as you said drop the default case,
there will be no problem. But really to do that?
Since Jason hasn't spoken up, I suggest you follow Dan's recommendation
and insert a patch to drop the default case before your error rework.
I see, will drop the 'default' case in a separated patch.
Thanks.
Sorry for the late. I agree to drop the default. Even SOCK_RAW could not
work as stream and any type should be supported explicitly.
Thanks