#84 win32file.TransmitFile and ConnectEx patch

[ionel]

sample usage:

import win32file, socket, pywintypes
s = socket.socket()
f = file('somefile','rb')
s.connect(('192.168.111.128',10002))
ol = pywintypes.OVERLAPPED()
print win32file.TransmitFile(s, win32file._get_osfhandle(f.fileno()), 0, 0, ol, 0, "prepended data", "appended data")
length = win32file.GetOverlappedResult(s.fileno(), ol, 1)
print length

I hope I haven't done the wrong thing but for the sake of simplicity I'm providing a patch the includes the previous ConnectEx patch (hopefully it'll get accepted :)

[ionel]

the patch

[ionel]

Logged In: YES
user_id=1189761
Originator: YES

File Added: test_transmitfile.py

[ionel]

[Mark Hammond]

Logged In: YES
user_id=14198
Originator: NO

Thanks. Unfortunately, I'm going to need to research the ConnextEx stuff a little more to convince myself the arg handling is reasonable - but it will be accepted one way or another :) However, as TransmitFile is likely to be a much simpler patch, if you supplied a patch for that alone (including tests ideally - see test_win32file.py for stuff you can maybe borrow?) I could get it in much sooner (and it would therefore be possible to make build 211). Otherwise I'll wrap both this and your ConnectEx patches up when I find time to tweak.

Thanks again.

[ionel]

Logged In: YES
user_id=1189761
Originator: YES

I'd rather have them both in the same build. Also, on the arg handling thing I imagine you didn't like the addrinfo stuff that I did there. It might look like a mess but i have looked in the socketmodule.c from the python dist and there's nothing i can use from there (there is a getsockaddrarg that is used in the connects, it does about the same thing that i do but on a larger _messy_ scale with more cases like unix sockets that don't apply here).

Um, I did attach a test_transmitfile.py - should that code really be in test_win32file.py ?

[Mark Hammond]

Logged In: YES
user_id=14198
Originator: NO

Sorry for the confusion (and for missing the test!) To be clear, the "problem" is simply my lack of cycles to try and digest what exactly the API allows for versus what your patch does. I'm not suggesting anything you are doing is wrong, just that its not immediately obvious to me (hence my comments re TransmitFile, which a patch for *is* likely to be "obvious" - but as you mentioned you combined them, I didn't look closely). You are correct though that the arg handling is my primary concern, and if you are really keen to get this in ASAP, would you be so kind as to mail the python-win32 list asking for other comments and pointing at this bug? There are a few people there who's opinion I would defer to - and they may have more time at the moment to have a look (I'm really hoping to get build 211 out any day now!)

Also, if you are intent on getting these in the same build, should I close the other patch?

Thanks!

[ionel]

Logged In: YES
user_id=1189761
Originator: YES

sure, close it.
I'll post a message on the list in a few moments.

[ionel]

Logged In: YES
user_id=1189761
Originator: YES

File Added: win32file.i-6.patch

[ionel]

[ionel]

Logged In: YES
user_id=1189761
Originator: YES

File Added: win32file.i-7.patch

[Roger Upole]

Logged In: YES
user_id=771074
Originator: NO

Regarding the way getaddrinfo is called, it appears that the
family, socket type, and protocol are hardcoded. Shouldn't it be
using the values from the Python socket object, since these are
specified when it's created ?

Also, Unicode is accepted for the host, but not the port. This
isn't a show stopper, but it would be better to be consistent.

[ionel]

revised patch

[ionel]

Logged In: YES
user_id=1189761
Originator: YES

File Added: win32file.i-8.patch

[ionel]

Logged In: YES
user_id=1189761
Originator: YES

File Added: win32file.i-9.patch

[ionel]

Fixed the return value for TransmitFile (returns a pending/succes or raise exception)

[Roger Upole]

There's another possible issue with the way getaddrinfo is used.
It returns a linked list of structures, rather than a single address.
This leaves no way to select which sockaddr will be passed to
ConnectEx. It might be better to accept a tuple as produced by
the builtin socket.getaddrinfo() so that the caller can choose.

[ionel]

I would rather not do that - too much parameter grunt work.

I would rather get the getsockaddrarg implementation from the python's socketmodule and plug it in here. What do you think ?

[Mark Hammond]

Even if you borrowed Python's getaddrinfo, wouldn't you still be in the situation that Roger mentions - getaddrinfo() natively returns a pointer to a linked list, while Python's implementation returns a Python list - which binding in either of the lists are you going to use? It appears you will always use the first?

getaddrinfo() returns a list of tuples, which each tuple being fairly simple to unpack (ints and strings IIUC). So if you just accept one of these tuples it should be quite easy to unpack this into an addrinfo struct - just a few lines, and much simpler than either the current code or trying to borrow and adapt Python's implementation.

I was about to have a go at modifying your patch to do that (there might be something I don't understand, so that was the best way to find out :), but then noticed that there seems to be no test for ConnectEx() - do you have any code available we can use as a test case for that? Otherwise it will be impossible for me to verify it actually works.

Thanks

[ionel]

File Added: test_connectex.py

[ionel]

tests for connectex

[ionel]

No, I don't mean borrow the getaddrinfo implementation in the python socket module, rather reuse the getsockaddrarg implementation in socketmodule.c and keep functionality in line with whatever is in the python standard library. I don't see anywhere in the socket module any connect method that takes a sockaddr struct as an argument so all this idea of passing a sockaddr of your own choosing seems unnatural.

[Mark Hammond]

Thanks for the clarification and update to the test. It still looks like a *lot* of code to duplicate, and code that will not be easy to keep up-to-date given all the platform-specific #ifdef's we'd probably want to remove. If we can avoid that, with the only cost being that ConnectEx takes one "unnatural" arg, I'm still leaning towards "practicality beats purity" in this case. ConnextEx is somewhat "unnatural" in its own right anyway.

I'd like to make sure I understand though (I've never done much serious socket work in case you can't tell ;) - the "unnatural" nature would specifically mean the code would change from doing:

win32file.ConnectEx(s2, self.addr, ol, "some expected request")
to something along the lines of:
win32file.ConnectEx(s2, socket.getaddrinfo(*self.addr)[0], ol, "some expected request")

Is that correct, or is there still something I'm missing?

[ionel]

getaddrinfo returns a tuple (family, socktype, proto, canonname, sockaddr).
sockaddr is the useful part here, that's what should be used (other parts of the tuple do not make much sense, and it doesn't feel exactly brilliant to implement an pattern matching algorithm over family/socktype/proto in a mere ConnectEx call wrapper). It needs to be encoded in a sockaddr struct.

However, the encoding routine might just be the getaddrinfo we don't like - since the address is in IP dotted address format, we'll get the exact sockaddr we want.
Looking at the problem like this all this discussion looks like nonsense since if you want control of whatever address family ConnectEx will use you can just pass the real address instead of a hostname to ConnectEx.

I hope this clarifies things.

[Mark Hammond]

How many families do you think we will need support for? If we just use AF_INET and maybe AF_INET6, and don't try to perform name resolution in ConnectEx (ie, stick closer to the API itself rather than "adding value" on top of it), then it seems the amount of code duplicated could indeed be quite small - 20 lines or so - as those structures are simple to unpack. Would that be reasonable?