[Qemu-devel] Re: [V5 PATCH 8/8] virtio-9p: Chroot environment for other functions

[Stefan Hajnoczi <stefanha@gmail.com>]

On Wed, Feb 16, 2011 at 12:23 PM, M. Mohan Kumar <mohan@in.ibm.com> wrote:
> +/*
> + * Returns file descriptor of dirname(path)
> + * This fd can be used by *at functions
> + */
> +static int get_dirfd(FsContext *fs_ctx, const char *path)
> +{
> +    V9fsFileObjectRequest request;
> +    int fd, error = 0;
> +    char *dpath = qemu_strdup(path);
> +
> +    fill_request(&request, dirname(dpath), NULL);
> +    request.data.type = T_OPEN;
> +    fd = v9fs_request(fs_ctx, &request, &error);
> +    if (fd < 0) {
> +        errno = error;
> +    }

man errno says:
"a function that succeeds is allowed to change errno"

Therefore you can't safely use errno to stash your error value and
call any code outside your control because it may overwrite errno.
Obliging callers to take these precautions is a bad idea because they
will forget.

The main feedback I have for this entire patchset is that these
functions introduce layers and layers of weird error handling.  A
large fraction of the code is just propagating error codes.  Sometimes
errno is used, sometimes an int * argument is used, sometime a
positive errno is returned although QEMU code normally returns a
negative errno.  Then the fd_info struct has a flag to indicate the fd
is invalid when we could just set it to -1 to indicate this.  It's
messy and hard for someone else to modify later without introducing
error handling bugs.

The method that fits most with QEMU's codebase is:
int foo(...) /* returns 0 on success, -errno on failure */

If you can refactor the code to unify error handling it will require
fewer lines and be simpler.

Stefan