[sheepdog] [PATCH] sheepdog: Set error when connection fails

[Jeff Cody]

---
block/sheepdog.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/block/sheepdog.c b/block/sheepdog.c
index fb9203e..7e889ee 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -608,6 +608,7 @@ static int connect_to_sdog(BDRVSheepdogState *s, Error **errp)
qemu_set_nonblock(fd);
} else {
fd = -EIO;
+ error_setg(errp, "Failed to connect to sheepdog server");
}

A bit odd that we don't just return right away in this function after a
failed called to socket_connect(), but that has nothing to do with your
patch.

return fd;
--
2.9.3

[Jeff Cody]

---
block/sheepdog.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/block/sheepdog.c b/block/sheepdog.c
index fb9203e..7e889ee 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -608,6 +608,7 @@ static int connect_to_sdog(BDRVSheepdogState *s, Error **errp)
qemu_set_nonblock(fd);
} else {
fd = -EIO;
+ error_setg(errp, "Failed to connect to sheepdog server");
}
return fd;
--
2.9.3

Thanks,

Applied to my block branch:

git://github.com/codyprime/qemu-kvm-jtc block

-Jeff

[Daniel P. Berrange]

---
block/sheepdog.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/block/sheepdog.c b/block/sheepdog.c
index fb9203e..7e889ee 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -608,6 +608,7 @@ static int connect_to_sdog(BDRVSheepdogState *s, Error **errp)
qemu_set_nonblock(fd);
} else {
fd = -EIO;
+ error_setg(errp, "Failed to connect to sheepdog server");
}

This doesn't make much sense to me. The lines just above the
diff context have this:

fd = socket_connect(s->addr, errp, NULL, NULL);

socket_connect should have already reported an error on "errp"
in the scenario that 'fd == -1'. So AFAICT the new error_setg is
just throwing away the real detailed error message in favour of
a generic message.

So I'm puzzelled why we need to change anything - error reporting
should already be working fine.

Regards,
Daniel

[Jeff Cody]

Post by Daniel P. Berrange

---
block/sheepdog.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/block/sheepdog.c b/block/sheepdog.c
index fb9203e..7e889ee 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -608,6 +608,7 @@ static int connect_to_sdog(BDRVSheepdogState *s, Error **errp)
qemu_set_nonblock(fd);
} else {
fd = -EIO;
+ error_setg(errp, "Failed to connect to sheepdog server");
}

This doesn't make much sense to me. The lines just above the
fd = socket_connect(s->addr, errp, NULL, NULL);
socket_connect should have already reported an error on "errp"
in the scenario that 'fd == -1'. So AFAICT the new error_setg is
just throwing away the real detailed error message in favour of
a generic message.
So I'm puzzelled why we need to change anything - error reporting
should already be working fine.

Indeed, you are right. (Dequeuing patch)

It would also make more sense to check fd after the socket_connect() call
and return error then, rather than keep checking fd throughout the rest of
the function.

-Jeff

[Daniel P. Berrange]

Post by Jeff Cody

Post by Daniel P. Berrange

---
block/sheepdog.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/block/sheepdog.c b/block/sheepdog.c
index fb9203e..7e889ee 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -608,6 +608,7 @@ static int connect_to_sdog(BDRVSheepdogState *s, Error **errp)
qemu_set_nonblock(fd);
} else {
fd = -EIO;
+ error_setg(errp, "Failed to connect to sheepdog server");
}

This doesn't make much sense to me. The lines just above the
fd = socket_connect(s->addr, errp, NULL, NULL);
socket_connect should have already reported an error on "errp"
in the scenario that 'fd == -1'. So AFAICT the new error_setg is
just throwing away the real detailed error message in favour of
a generic message.
So I'm puzzelled why we need to change anything - error reporting
should already be working fine.

Indeed, you are right. (Dequeuing patch)
It would also make more sense to check fd after the socket_connect() call
and return error then, rather than keep checking fd throughout the rest of
the function.

Yeah that would make it much more obvious that the error reporting is
correct.

Regards,
Daniel

[Kevin Wolf]

Post by Daniel P. Berrange

---
block/sheepdog.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/block/sheepdog.c b/block/sheepdog.c
index fb9203e..7e889ee 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -608,6 +608,7 @@ static int connect_to_sdog(BDRVSheepdogState *s, Error **errp)
qemu_set_nonblock(fd);
} else {
fd = -EIO;
+ error_setg(errp, "Failed to connect to sheepdog server");
}

This doesn't make much sense to me. The lines just above the
fd = socket_connect(s->addr, errp, NULL, NULL);
socket_connect should have already reported an error on "errp"
in the scenario that 'fd == -1'.

By the way, am I the only one who thinks that having errp anywhere else
than as the last argument is bad style? I can easily see myself missing
that this functions sets it because the last argument is NULL.

Kevin

[Jeff Cody]

Post by Kevin Wolf

Post by Daniel P. Berrange

---
block/sheepdog.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/block/sheepdog.c b/block/sheepdog.c
index fb9203e..7e889ee 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -608,6 +608,7 @@ static int connect_to_sdog(BDRVSheepdogState *s, Error **errp)
qemu_set_nonblock(fd);
} else {
fd = -EIO;
+ error_setg(errp, "Failed to connect to sheepdog server");
}

This doesn't make much sense to me. The lines just above the
fd = socket_connect(s->addr, errp, NULL, NULL);
socket_connect should have already reported an error on "errp"
in the scenario that 'fd == -1'.

By the way, am I the only one who thinks that having errp anywhere else
than as the last argument is bad style? I can easily see myself missing
that this functions sets it because the last argument is NULL.

No, you are not - that is precisely why I missed it in my review.


-Jeff