Jump to content


Photo

merge requests for PLi's git


  • Please log in to reply
1748 replies to this topic

Re: merge requests for PLi's git #321 betacentauri

  • PLi® Core member
  • 7,185 posts

+323
Excellent

Posted 9 November 2013 - 00:53

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


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

Xtrend ET-9200, ET-8000, ET-10000, OpenPliPC on Ubuntu 12.04

Re: merge requests for PLi's git #322 Erik Slagter

  • PLi® Core member
  • 46,969 posts

+541
Excellent

Posted 9 November 2013 - 10:15

Nice work!


* Wavefrontier T90 with 28E/23E/19E/13E via SCR switches 2 x 2 x 6 user bands
I don't read PM -> if you have something to ask or to report, do it in the forum so others can benefit. I don't take freelance jobs.
Ik lees geen PM -> als je iets te vragen of te melden hebt, doe het op het forum, zodat anderen er ook wat aan hebben.


Re: merge requests for PLi's git #323 pieterg

  • PLi® Core member
  • 32,766 posts

+245
Excellent

Posted 9 November 2013 - 11:12

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

Re: merge requests for PLi's git #324 betacentauri

  • PLi® Core member
  • 7,185 posts

+323
Excellent

Posted 9 November 2013 - 11:22

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.

Xtrend ET-9200, ET-8000, ET-10000, OpenPliPC on Ubuntu 12.04

Re: merge requests for PLi's git #325 Erik Slagter

  • PLi® Core member
  • 46,969 posts

+541
Excellent

Posted 9 November 2013 - 13:05

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?


* Wavefrontier T90 with 28E/23E/19E/13E via SCR switches 2 x 2 x 6 user bands
I don't read PM -> if you have something to ask or to report, do it in the forum so others can benefit. I don't take freelance jobs.
Ik lees geen PM -> als je iets te vragen of te melden hebt, doe het op het forum, zodat anderen er ook wat aan hebben.


Re: merge requests for PLi's git #326 pieterg

  • PLi® Core member
  • 32,766 posts

+245
Excellent

Posted 9 November 2013 - 13:12

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.

Re: merge requests for PLi's git #327 Erik Slagter

  • PLi® Core member
  • 46,969 posts

+541
Excellent

Posted 9 November 2013 - 13:19

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


* Wavefrontier T90 with 28E/23E/19E/13E via SCR switches 2 x 2 x 6 user bands
I don't read PM -> if you have something to ask or to report, do it in the forum so others can benefit. I don't take freelance jobs.
Ik lees geen PM -> als je iets te vragen of te melden hebt, doe het op het forum, zodat anderen er ook wat aan hebben.


Re: merge requests for PLi's git #328 theparasol

  • Senior Member
  • 4,157 posts

+198
Excellent

Posted 9 November 2013 - 15:44

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

 

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


@Camping: ZGemma H.2S, Technisat Multytenne 4-in-1 @Home: Edision Mini 4K, Wave Frontier T55, EMP Centauri EMP DiSEqC 8/1 switch, 4x Inverto Ultra Black single LNB


Re: merge requests for PLi's git #329 littlesat

  • PLi® Core member
  • 57,062 posts

+698
Excellent

Posted 9 November 2013 - 15:55

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

WaveFrontier 28.2E | 23.5E | 19.2E | 16E | 13E | 10/9E | 7E | 5E | 1W | 4/5W | 15W


Re: merge requests for PLi's git #330 zakalibit

  • Senior Member
  • 51 posts

+5
Neutral

Posted 9 November 2013 - 18:26

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() :)



Re: merge requests for PLi's git #331 pieterg

  • PLi® Core member
  • 32,766 posts

+245
Excellent

Posted 9 November 2013 - 18:51

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)

Re: merge requests for PLi's git #332 MiLo

  • PLi® Core member
  • 14,055 posts

+298
Excellent

Posted 9 November 2013 - 19:21

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.
Real musicians never die - they just decompose

Re: merge requests for PLi's git #333 zakalibit

  • Senior Member
  • 51 posts

+5
Neutral

Posted 9 November 2013 - 21:50

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.  



Re: merge requests for PLi's git #334 zakalibit

  • Senior Member
  • 51 posts

+5
Neutral

Posted 10 November 2013 - 11:26

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.


Re: merge requests for PLi's git #335 Erik Slagter

  • PLi® Core member
  • 46,969 posts

+541
Excellent

Posted 10 November 2013 - 11:39

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.


* Wavefrontier T90 with 28E/23E/19E/13E via SCR switches 2 x 2 x 6 user bands
I don't read PM -> if you have something to ask or to report, do it in the forum so others can benefit. I don't take freelance jobs.
Ik lees geen PM -> als je iets te vragen of te melden hebt, doe het op het forum, zodat anderen er ook wat aan hebben.


Re: merge requests for PLi's git #336 betacentauri

  • PLi® Core member
  • 7,185 posts

+323
Excellent

Posted 10 November 2013 - 11:58

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?
Xtrend ET-9200, ET-8000, ET-10000, OpenPliPC on Ubuntu 12.04

Re: merge requests for PLi's git #337 zakalibit

  • Senior Member
  • 51 posts

+5
Neutral

Posted 10 November 2013 - 12:08

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



Re: merge requests for PLi's git #338 Erik Slagter

  • PLi® Core member
  • 46,969 posts

+541
Excellent

Posted 10 November 2013 - 12:23

@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.


* Wavefrontier T90 with 28E/23E/19E/13E via SCR switches 2 x 2 x 6 user bands
I don't read PM -> if you have something to ask or to report, do it in the forum so others can benefit. I don't take freelance jobs.
Ik lees geen PM -> als je iets te vragen of te melden hebt, doe het op het forum, zodat anderen er ook wat aan hebben.


Re: merge requests for PLi's git #339 betacentauri

  • PLi® Core member
  • 7,185 posts

+323
Excellent

Posted 10 November 2013 - 13:09

@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.

Xtrend ET-9200, ET-8000, ET-10000, OpenPliPC on Ubuntu 12.04

Re: merge requests for PLi's git #340 Erik Slagter

  • PLi® Core member
  • 46,969 posts

+541
Excellent

Posted 10 November 2013 - 13:11

Thanks, very much appreciated!


* Wavefrontier T90 with 28E/23E/19E/13E via SCR switches 2 x 2 x 6 user bands
I don't read PM -> if you have something to ask or to report, do it in the forum so others can benefit. I don't take freelance jobs.
Ik lees geen PM -> als je iets te vragen of te melden hebt, doe het op het forum, zodat anderen er ook wat aan hebben.



46 user(s) are reading this topic

0 members, 45 guests, 0 anonymous users


    Bing (1)