These constants were added at different times, so they should be checked for individually, like:

#ifdef RWF_DSYNC
    if (PyModule_AddIntConstant(m, "RWF_DSYNC", RWF_DSYNC)) return -1;
#endif
#ifdef RWF_HIPRI
    if (PyModule_AddIntConstant(m, "RWF_HIPRI", RWF_HIPRI)) return -1;
#endif
...

Also EOPNOTSUPP flag might be important to add?

That's already available as errno.EOPNOTSUPP.

You use HAVE_PREADV2, but I don't see anything to actually set it. Did you forget to update configure.ac?

API question: preadv2 only exists because regular preadv was missing a flags argument, and in C the only way to add an argument is to make a new function. In python we don't have that problem. So an alternative would be to make this os.preadv(fd, buffers, offset[, flags]), and have it call either preadv or preadv2 as appropriate. (preadv2(..., flags=0) is equivalent to preadv.)

I kind of like this better, since (a) exposing preadv is probably a good idea, even on systems that don't have preadv2, (b) it reduces clutter in os. OTOH the same thing happened for pipe and pipe2, and it looks like we exposed those as two independent functions in os, so ¯\(ツ)/¯

https://www.gnu.org/software/libc/manual/html_mono/libc.html#index-preadv2 seems to support @njsmith statement.

@njsmith Thanks for the comment! CPython only exposes pread and readv and currently, it does not expose preadv. Is your suggestion to implement preadv2 as preadv and call the former if flags are provided and the later (preadv) in ther case?

I am asking this because initially I thought you were saying that preadv was available and we should reuse the function

Right, I'm doing the classic pull request reviewer move of trying to trick you into doing extra work while you're there :-). We don't currently expose preadv, but it's obviously a useful thing, so I'm suggesting you add a preadv wrapper, and then add preadv2 support to that. If we add preadv2 now alone, then we'll probably end up adding preadv later, and thus have a messier api in the end.

BTW, it's probably simpler to structure the code like (forgive my mixed Python/c pseudocode):

#if defined(HAVE_PREADV) || defined (HAVE_PREADV2)
def preadv(fd, offset, buffers, flags=0):
#ifdef HAVE_PREADV2
    return preadv2(...)
#else
    if flags != 0: raise ...
    return preadv(...)
#endif

Since preadv2 has a superset of the functionality of preadv, when it's available you can use it for all flags values, including 0.

Style nit: I'm not sure why, but the convention in C is that preprocessor directives are never indented, so like:

    if (iov_setup(&iov, &buf, buffers, cnt, PyBUF_WRITABLE) < 0)
        return -1;
#ifdef HAVE_PREADV2
    do {

(and similarly elsewhere).

If we exit early (e.g. due to flags != 0), then this cleanup function doesn't get called. Is that OK?

I have corrected it in 4ddbca9. I could not use easily a goto label to have a clean exit section due to the check for async error in the signal handler so I just clean before exiting if flags != 0.

You put asterisks around flags below and I think the first offset should have the name because this is referring to the argument.

Corrected in c7b7285

I don't know the answer to this, but should it be flags=None with no spaces?

Actually, I just realized this is implemented in C. I can't really comment on that code, but you have it as flags=0 in the C module. Is None and 0 the same thing?

You are right. The None was because a previous declaration before switching to the default suggested by @njsmith. It should be flags=0. Corrected in c7b7285.

I noticed you didn't duplicate the definition of fd and buffers from readv. I think most of docs explain all the parms, even if they are duplicated from another section.

I has hessitant to duplicate because readv is the function described inmediately before this one but I suppose that consistency makes sense. Thanks.

Corrected in c7b7285

Unix: do you mean Linux?

"FreeBSD (version 6.0 and newer)" may be written "FreeBSD 6 and newer".

Done in 47f65b5

of which flags? write an explicit list here.

Done in 47f65b5

nitpick: add spaces (see PEP 7): "if (flags != 0) {".

Done in 47f65b5

You misused the API. It should be something like: argument_unavailable_error("pwritev", "flags");

Sorry, my fault. Is 2AM here and I clearly should be sleeping. 😪

I would move this paragraph to the end, before Availability, and simplify it to one sentence, something like: "The :c:func:pwritev2 function is required to pass flags."

I would prefer to not mention NotImplementedError, it's not documlented for other functions.

Maybe mention in which Linux version is it was added (4.14?).

Hum, offset is not documented. You may just write: "writes the contents of buffers to file descriptor fd at offset offset."

nitpick: just "since Linux 4.7" (no "and newer") or "available on Linux 4.7 and newer".

nitpick: I would move this sentence before describing on which Linux platforms version it is available.

may be => can be

os.preadv2? I guess that you mean the C function preadv2, no? os.preadv2 doesn't exist.

I suggest "The C :c:func:preadv2 function is required ...".

posix.preadv2 doesn't exist. I suggest "test needs posix.RWF_HIPRI".

I also suggest to rename the test to test_preadv_flags() to be closer to the Python API than the C internals.

I would prefer to test writing multiple buffers than a single buffer. Maybe write [b'test1', b'tt2', b't3']?

I would prefer to write explicitly the first 2 bytes, but then seek back at offset 0 before calling os.pwritev(). I don't want to test in Python the behaviour of pwritev() for non-offset offset with an empty file. Python should test its binding, not the exact behaviour of the OS.

For example:
write(b'XX')
seek(0)
pwrite(..., offset=2)
assert read() == b'XX...'

ditto, rename to test_pwritev_flags() and change the skip message to not mention a function that doesn't exist.

PEP 7 coding style: add { ... }

ditto: add { ... }

a single empty line is enough.

No, there is no :func:~os.pwritev2 function, stop creating links to it.

I suggested "The C function :func:pwritev2", or you can just write pwrite2().

I'm not sure that ", which modifies the behavior on a per-call basis based on the value of the flags argument." is needed. Nobody expects a flag passed to a function to have an effect on following calls. And the usage of pwrite2() doesn't mean that the caller must pass a non-zero flags. This sentence is confusing.

ditto, don't write ":func:~os.pwritev2 function" but "C :c:func:pwritev2 function" or just pwrite2().

Note the "c" prefix before "func".

"The :func:~os.pwritev2 function is required to pass flags. pwritev2 and flags different than zero are available since Linux version 4.7."

I'm not sure that the Python has to mention pwritev2(). It's confusing and useless. I don't think that the documentation of other os function lists each internal called C functions.

I suggest something simpler like: "Using non-zero flags requires Linux 4.7 or newer."

ditto: remove this empty line

PEP 8: add a space after ","

I suggest: Add os.preadv() and os.pwritev() functions.

Again, I don't think that it's needed to mention the underlying C functions.

Wait... is this description correct? it seems to be copied from two other functions?