On 02/06/2015 01:56 PM, Eric Blake wrote:
On 02/06/2015 09:41 AM, Stefan Berger wrote:
@@ -214,6 +215,12 @@ virCommandReorderFDs(virCommandPtr cmd)
if (!cmd || cmd->has_error || !cmd->npassfd)
return;
+ if ((cmd->flags & VIR_EXEC_FIXED_FDS)) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("The fds are fixed and cannot be reordered"));
+ goto error;
+ }
Okay, so you are enforcing that a caller can't pick two conflicting fd
passing methods on the same virCommand, but my point remains that the
testsuite should prove we can hit this error case, to avoid regressions.
Or we should redo virCommand to ALWAYS rearrange fds in the child, and
make it possible for a caller passing an fd down to know what fd the
child will see it as.
That's the part I don't quite follow. We are passing indices via
/dev/fdset/%d, right? At the time we build up the command line, we need
to know the index that the fd will have. If we shuffle things around
later in a way that we cannot determine at the point we build up the
command line, how would this work then?
When you register an fd with virCommand, you would know at that point
what fd the child will see (regardless of what the fd number was in the
parent); ideally, the first fd you register will be fd 3 in the guest,
the next one will be fd 4 in the guest. I'm further proposing that we
number our /dev/fdset/%n after the first fd we place in the set (that
is, we know the guest will see fd 3, so place it in /dev/fdset/3). The
only time an fdset needs more than one member is if we pass multiple fds
pointing to the same file (a read-only fd and a read-write fd, for
example), but we don't have a use for that yet.
We are not passing all file descriptors to the child. So how would you
handle passing fd's 10,3,55 to the child? Do we pass empty slots for 4-9
and 11-54 then, like pass fd = -1, so we can pass indices 10,3, and 55
or something like that?
In my proposal, the parent's 10, 3, 55 become the child's 3, 4, 5. The
current code has the parent's 10 become the child's 10, with closed fds
in between, except when using the newer code that packs things and
merely passes an environment variable to say how many were packed. I'm
proposing that we just always pack things, and make it easier to know
what number we packed into, and that the parent then build the command
line around the packed number instead of around the parent's number.
I see. I guess a modified virCommandFDSet may then look like this:
/*
* virCommandFDSet:
* @fd: FD to be put into @set
* @set: the set
* @set_size: actual size of @set
*
* This is practically generalized implementation
* of FD_SET() as we do not want to be limited
* by FD_SETSIZE.
*
* Returns: the target filedescriptor on success,
* -1 on usage error,
* -ENOMEM on OOM // -> no returns negative errno to not
collide with file descriptor
*/
static int
virCommandFDSet(virCommandPtr cmd,
int fd,
unsigned int flags)
{
if (!cmd || fd < 0)
return -1;
if (virCommandFDIsSet(cmd, fd))
return 0;
if (VIR_EXPAND_N(cmd->passfd, cmd->npassfd, 1) < 0)
return -ENOMEM;
cmd->passfd[cmd->npassfd - 1].fd = fd;
cmd->passfd[cmd->npassfd - 1].flags = flags;
return cmd->npassfd - 1 + 3; // the filedescriptor given to the
child; base is '3'
}
virCommandReorderFDs is not used on WIN32 -- that would then mean that
the fdsets could not be used on WIN32 or a different return value would
have to be given here. Like
return cmd->npassfd -1
on WIN32 ?
Stefan
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
