On Thursday 03 March 2011 5:39:55 pm Stefan Hajnoczi wrote:
> > + v9fs_write_request(fs_ctx->chroot_socket, request);
> > + fd = v9fs_receivefd(fs_ctx->chroot_socket, &sock_error);
> > + if (fd < 0 && sock_error) {
> > + fs_ctx->chroot_ioerror = 1;
> > + }
>
> chroot_ioerror, sock_error, and their FdInfo bits are redundant code.
> The chroot child could just exit on error and the parent would get
> errors when writing the request here, which is the same effect as
> manually returning -EIO in this function. There is no need to
> introduce variables and bits to communicate this failure mode.
>
> Once that simplification has been made, FdInfo becomes just an -errno
> value to pass back the result of open(2) and friends. That means we
> can completely drop FdInfo and the fi_fd field which doesn't actually
> hold a useful fd value for the QEMU process but just a -errno.
>
But we need to pass the fd to qemu process, so it could not be -errno. When
fd >= 0 its a valid fd otherwise its a errno.
> Instead send back only an int -errno return code from the chroot child.
>
> You mentioned wanting to distinguish between socket errors and
> blocking syscall errors but since there isn't any real error handling
> that makes use of that information (and I'm not sure there is a good
> error handling strategy that could be used), this is all just adding
> complexity.
>
> > +/* Helper routine to fill V9fsFileObjectRequest structure */
> > +static int fill_fileobjectrequest(V9fsFileObjectRequest *request,
> > + const char *path, FsCred *credp)
> > +{
> > + if (strlen(path) > PATH_MAX) {
> > + return -ENAMETOOLONG;
> > + }
>
> Off-by-one error. It should strlen(path) >= PATH_MAX to account for
> the NUL terminator.
>
Ok, I Will change!
> > + memset(request, 0, sizeof(*request));
> > + request->data.path_len = strlen(path);
> > + strcpy(request->path.path, path);
> > + if (credp) {
> > + request->data.mode = credp->fc_mode;
> > + request->data.uid = credp->fc_uid;
> > + request->data.gid = credp->fc_gid;
> > + request->data.dev = credp->fc_rdev;
> > + }
> > + return 0;
> > +}
> > +
> > +static int passthrough_open(FsContext *fs_ctx, const char *path, int
> > flags) +{
> > + V9fsFileObjectRequest request;
> > + int fd;
> > +
> > + fd = fill_fileobjectrequest(&request, path, NULL);
> > + if (fd < 0) {
> > + errno = -fd;
>
> Please don't use errno to communicate errors back. In this function
> it would be perfectly fine to return fd here since it is already a
> -errno.
>
> It's not safe to use errno since it's value can be lost by calling any
> external function - its value may be modified even if no error occurs.
> I quoted from the errno(3) man page in a previous review:
> "a function that succeeds *is* allowed to change errno"
>
> > + return -1;
> > + }
> > + request.data.flags = flags;
> > + request.data.type = T_OPEN;
> > + fd = v9fs_request(fs_ctx, &request);
> > + return fd;
> > +}
> >
> > static int local_lstat(FsContext *fs_ctx, const char *path, struct stat
> > *stbuf) {
> > @@ -138,14 +175,27 @@ static int local_closedir(FsContext *ctx, DIR *dir)
> > return closedir(dir);
> > }
> >
> > -static int local_open(FsContext *ctx, const char *path, int flags)
> > +static int local_open(FsContext *fs_ctx, const char *path, int flags)
> > {
> > - return open(rpath(ctx, path), flags);
> > + if (fs_ctx->fs_sm == SM_PASSTHROUGH) {
> > + return passthrough_open(fs_ctx, path, flags);
> > + } else {
> > + return open(rpath(fs_ctx, path), flags);
> > + }
> > }
> >
> > -static DIR *local_opendir(FsContext *ctx, const char *path)
> > +static DIR *local_opendir(FsContext *fs_ctx, const char *path)
> > {
> > - return opendir(rpath(ctx, path));
> > + if (fs_ctx->fs_sm == SM_PASSTHROUGH) {
> > + int fd;
> > + fd = passthrough_open(fs_ctx, path, O_DIRECTORY);
> > + if (fd < 0) {
> > + return NULL;
> > + }
> > + return fdopendir(fd);
> > + } else {
> > + return opendir(rpath(fs_ctx, path));
> > + }
>
> Perhaps we should use a local_operations struct or similar function
> pointer table instead of adding fs_ctx->fs_sm conditionals everywhere.
That would have more code duplication when compared to additional
conditionals. i.e, We need to create local_passthrough_opendir,
local_passthrough_open, .. etc.
>
> Also it would be neat to reuse the local implementations on the chroot
> child side instead of instead of splitting code paths, but I'm not
> sure whether that is possible.
I am not sure what do you mean by reusing the virtio-9p-local.c implementation
in chilld side. That may involve breaking down existing local functions?
----
M. Mohan Kumar
