Re: [libvirt] [PATCH 4/7] virfile: Adjust error path for virFileOpenForked
On 01/28/2015 03:25 PM, John Ferlan wrote:
Rather than have a dummy waitpid loop and return of the failure
status
from recvfd, adjust the logic to save the recvfd error & fd and then
in priority order:
if waitpid failed, use that
if waitpid succeeded, but the child reported failure, use that
regardless of recvfd status
if waitpid succeeded and reported success, but recvfd failed with
anything other than EACCES or ENOTCONN, use recvfd's result
otherwise, waitpid and recvfd succeeded, return the fd
NOTE: Original logic to retry the open and force owner mode was
"documented" as only being attempted if we had already tried opening
with the fork+setuid, but checked flags vs. VIR_FILE_OPEN_NOFORK which
is counter to how we would get to that point. So that code was removed.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/util/virfile.c | 67 +++++++++++++++++++++++++++++++-----------------------
1 file changed, 39 insertions(+), 28 deletions(-)
+ /*
+ * if waitpid succeeded, but the child reported failure, use that
+ * regardless of recvfd status
+ */
+ if (WIFEXITED(status) && WEXITSTATUS(status) != 0) {
+ ret = WEXITSTATUS(status);
+ virReportSystemError(ret,
waitpid reporting !WIFEXITED(status) is a case of the child reporting
failure (well, reporting abnormal exit due to signal), so it might
simplify things if we just do[1]:
if (!WIFEXITED(status) || WEXITSTATUS(status) != 0) {
except that then you have to distinguish between abnormal exit vs. exit
encoding a known errno value when constructing the error message.
+
+ /* if waitpid succeeded and reported success, but recvfd failed with
+ * anything other than EACCES or ENOTCONN, use recvfd's result
+ */
+ if (WIFEXITED(status) && WEXITSTATUS(status) == 0 && fd < 0
&&
+ !(recvfd_errno == EACCES || recvfd_errno == ENOTCONN)) {
+ virReportSystemError(recvfd_errno,
+ _("failed recvfd for child creating
'%s'"),
+ path);
+ return -recvfd_errno;
+ }
Hmm. It may be sufficient to just treat ALL recvfd failures as the exit
status if we get here, since if recvfd failed, then fd == -1...
+
+ /* Some sort of abnormal child termination */
+ if (!WIFEXITED(status) || fd == -1) {
+ VIR_FORCE_CLOSE(fd);
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("child exited abnormally, failed to create
'%s'"),
+ path);
+ return -EACCES;
...and we would be failing anyways, but losing the errno value from
recvfd. And if you change point [1] above to catch abnormal exit, then
this entire if branch becomes dead.
So how about:
- if waitpid failed, use that errno value
- waitpid succeeded, but if the child exited abnormally, report failure
(not sure what errno value to use here)
- waitpid succeeded, but if the child reported non-zero status, report
failure (use the errno value that the child encoded into exit status)
- waitpid succeeded, but if recvfd failed, report recvfd_errno
- waitpid and recvfd succeeded, use the fd
Sorry for making you churn on this, given my first-round off-list
thoughts on the matter, but hopefully it turns out a little easier to read.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachments:
- signature.asc (application/pgp-signature — 604 bytes)