Re: [RFC] atomic open(..., O_CREAT | ...)

[Miklos Szeredi]

> We've already got a patch that does this, and that I'm queueing up for
> inclusion.

Cool!

> http://client.linux-nfs.org/Linux-2.6.x/2.6.12/linux-2.6.12-63-open_file_intents.dif

Comments:

>  /*
> + * Open intents have to release any file pointer that was allocated
> + * but not used by the VFS.
> + */
> +void path_release_open_intent(struct nameidata *nd)
> +{
> +	if ((nd->flags & LOOKUP_OPEN) && nd->intent.open.file != NULL) {
> +		fput(nd->intent.open.file);

I think you should consider adding this:

+		if (!IS_ERR(nd->intent.open.file))
+			fput(nd->intent.open.file);

so the filesystem can delay returning the error from the open
operation until the other errors have been sorted out by the lookup
code.

> +		nd->intent.open.file = NULL;

Why is this NULL assignment needed?  nd will not be used after this.

> +	}
> +	path_release(nd);
> +}
> +
> 



> As for the "orig flags" thing. What is the point of that?

dentry_open() needs the original open flags, not the transformed ones
stored in intent.open.flags.

The behavior is slightly strange, since filp_open() calculates
namei_flags (which gets stored in intent.open.flags) so that an
O_ACCMODE of 3 is transformed into FMODE_READ | FMODE_WRITE.

But dentry_open() calculates filp->f_mode, so that O_ACCMODE of 3 is
transformed into zero.

This means that the (undocumented) access mode of 3 will require
read-write permission, but will allow neither read() nor write() on
the opened file.

If we want to keep this behavior, then the orig_flags field is needed.

Thanks,
Miklos
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/