1748 replies to this topic

[betacentauri]

Hi Alex,

I did part of the work. Here are 5 patches. Not everything is included (due to missing time). Can you please check, whether the patches are ok? I have only compiled them without error, but I did no tests.
You can exchange the From:.... line in the patches with your email address (well, they are your patches).

Attached Files

•  0001-eHttpStream-Add-m_-to-all-class-member-variables.patch.txt   5.76KB   4 downloads
•  0002-eHttpStream-improvements.patch.txt   13.09KB   4 downloads
•  0003-timedRead-Use-poll-instead-of-select.patch.txt   1.64KB   3 downloads
•  0004-Handle-non-unix-lines-r-n-line-breaks.patch.txt   824bytes   1 downloads
•  0005-eHttpStream-cosmetics.patch.txt   5.76KB   2 downloads

Edited by betacentauri, 9 November 2013 - 00:54.

[Erik Slagter]

Nice work!

[pieterg]

3. What are the advantages of using poll instead of select?
5. Several cosmetics are fixes for 2, these could be squashed/avoided?

[betacentauri]

To 3: zakalibit has to tell you.

Yes, it should be possible to squash the patches 2 and 5. But perhaps not everything. E.g. this has nothing to do with the new functionality:

@@ -107,8 +107,7 @@ int eHttpStream::openUrl(const std::string &url, std::string &newurl)
 		request.append("Authorization: Basic ").append(authorizationData).append("\r\n");
 	}
 	request.append("Accept: */*\r\n");
-	request.append("Connection: close\r\n");
-	request.append("\r\n");
+	request.append("Connection: close\r\n\r\n");
 
 	result = openHTTPConnection(m_streamSocket, request, hdr);
 	if (result < 0)

Edited by betacentauri, 9 November 2013 - 11:23.

[Erik Slagter]

3. What are the advantages of using poll instead of select?

I personally prefer poll as well, although it does not necessarily gives an improvement on the technical level. Maybe it's ok if the change is isolated as semi-cosmetic?

[pieterg]

I prefer the select syntax. But both use the same principle, so no performance difference. And no real reason to switch from one to another.

[Erik Slagter]

Personal taste I don't know what's usually is used in enigma2, I guess it makes sense to stick to that version.

[theparasol]

I found some more background to poll() and select()

 

http://daniel.haxx.s...-vs-select.html

[littlesat]

I hope at this tread will end to a good improved final solution....

[zakalibit]

FD_SET size that is used in select it is a compile time sized used a define (do not remember the exact name, probably FD_SET_SIZE), by default the size is 1024 most of the times, so if you want to monitor a socket with fd number bigger then 1023  with select you'll get an unpredictable behaviour, even a core, other then that there is no real advantage, btw I remember stracing an app at the time, I saw select() calling poll()

[pieterg]

the FD_SET size is at least the open file limit (1024 by default).
And about select calling poll, it often is the other way around (select has been around for longer than poll, several os-es implement poll with select).
In the end, both poll and select call the same poll operation in the kernel, for all filedescriptors. (but you don't get to see that with strace)

[MiLo]

The select() syntax is more convenient if your fd set changes on each call. The poll syntax is handy when the fd set remains unchanged.

Other than that, like P said, they end up calling the same kernel methods. So the difference is pure syntax.

[zakalibit]

the FD_SET size is at least the open file limit (1024 by default).
And about select calling poll, it often is the other way around (select has been around for longer than poll, several os-es implement poll with select).
In the end, both poll and select call the same poll operation in the kernel, for all filedescriptors. (but you don't get to see that with strace)

you're right strace does not show that, can't recall how did I see it may be it was gdb or pstack, the actual call was not the poll() call, but another as I remember some sort of _poll().

Actually it does not matter, if you like leave it as select(), I do not care that much, I did it distinctively, as I had to solve issues in production related to this, nobody is going to change limits on these boxes.  

[zakalibit]

Hi betacentauri, I have reviewed your patches, everything looks good, here is few points in the patches that I'd disagree:

 

1. you have increased the scope of some variables, that is  so much c era, in c++ you declare them right before they're used, I wont go in to polemics why it is better too have the smallest possible scope for a variable. 

 

and in c++  the following is preferred:

 

int i = value; or int i(value);

 

over:

int i;

i = value;

 

2. This is a matter of reference, but I still think one liner if statement looks better and it gives you more code on one screen page

 

3. You have changed some of the log messages in void eHttpStream::thread(),  in my opinion your messages are useless, I have spent hours looking at the logs trying to figure out what is going on in that code, I changed them for a reason. For example how do you distinguish a connection failure from too many redirects? And what is the mining of "Thread end connection", do you think this will tell you the right think when looking in the logs?

Edited by zakalibit, 10 November 2013 - 11:29.

[Erik Slagter]

I am not argueing with the principal points, but: betacentauri is adhering to the coding standard of enigma. We all have to comprise (me too!) to keep the code more or less readable for anyone.

[betacentauri]

Hi,

to 1: Yes, I like it the "old" style. I also don't want to discuss ;-)
@pli: What's your preferred style?

to 2: Sometimes I prefer one liner sometimes not. E.g. if (..) break; is ok. But if condition or statement is very long I prefer 2 lines.
@pli: What do you think about one liners?

to 3: I forgot to change the log messages.

With the feedback from Pli I can change the patches another time. I guess I can throw away patch 3. Or is somebody interested in it any longer?

[zakalibit]

you have asked for a review I did it I do not think that I can change anything, at least I voiced my opinion.

[Erik Slagter]

@betacentauri, one liners are "not preferred" (although personally I think they make sense sometimes). There are pro's and cons to having variable declarations in the midst of executable code, but afaik, overall in enigma, variable declarations are almost always done before code, so better stick to that, I guess.

 

Similarly I have to agree with pieterg that it's not good practise / not nice to change code that is working correctly, other than to make it adhere to the coding standard. If you were the one having written the code, you wouldn't like that too, I guess.

[betacentauri]

@zakalibit: I forgot to say, thanks for the review! And you can change something ;-) E.g. I'll add the log messages, which are fine.

@erik: Ok, I'll try to keep that in mind when adapting the patches.

Give me some days. I need to find some time to prepare the patches.

Edited by betacentauri, 10 November 2013 - 13:09.

[Erik Slagter]

Thanks, very much appreciated!