Unity-Technologies / qstat

New official qstat repository
Artistic License 2.0
121 stars 33 forks source link

Fix Teeworlds server query, #3 #4

Closed illwieckz closed 7 years ago

illwieckz commented 9 years ago

Hi, this fixes the Teeworlds server query, it tries with the 3 known getinfo packets.

I renamed the -tee argument to -tees since I will work next on the -teem Teeworlds master query.

Since Teeworlds support was broken since many years, there is probably nobody that uses the -tee arg today.

stevenh commented 9 years ago

Overall can you please update the formatting for the teeworld module as per the new style, which is based off: https://www.freebsd.org/cgi/man.cgi?query=style&sektion=9

An example of this can be found in the dirty bomb module.

illwieckz commented 9 years ago
-query_status_t deal_with_tee_packet( struct qserver *server, char *rawpkt, int pktlen )
+query_status_t deal_with_tee_packet( struct qserver *server, char *pkt, int pktlen )

This is still a raw packet please keep the old var name

That was because

-query_status_t deal_with_tee_packet( struct qserver *server, char *rawpkt, int pktlen )
+query_status_t deal_with_tee_packet( struct qserver *server, char *pkt, int pktlen )
-       char *pkt = rawpkt + 6;

Because the + 6 was not relevant anymore, I've only rename rawpkt this line to avoid renaming each occurrence in the whole file. So, I will rename pkt as rawpkt in the whole file.

Overall can you please update the formatting for the teeworld module as per the new style, which is based off: https://www.freebsd.org/cgi/man.cgi?query=style&sektion=9 An example of this can be found in the dirty bomb module.

OK. Good idea.

+ irelavent change, please remove from the commit.

Yes, unfortunately, I see that pointless thing after my pull request so I did not rewrite the commit to not pollute the git history and the github history (because the commit name contains the issue number, each commit rewrite can appear in the issue #3 thread).

I've already deleted this useless line, so it will disappear in my next commit (master server).

I will propose a clean pull request with server support fix + new master server when master support will be finished.

The master server support is well underway, but I need a little help, cf. https://github.com/multiplay/qstat/issues/5 :wink:

leepa commented 9 years ago

You can delete the line and commit --amend to update your pull request.

illwieckz commented 9 years ago

I rewritten the commit.

stevenh commented 9 years ago

Thanks for fixing the issues, appreciate half of them are legacy code, but best to fix them as your going through :)

stevenh commented 9 years ago

This pull request is for #3

illwieckz commented 9 years ago

Hi, what do you think about renaming tee.c to teeworlds.c and qstat args to -teeworldss and -teeworldsm ? I see that newer games use complete name. Since the Teeworlds server support was broken since many years and the Teeworlds master support was never written before, there will not be many people complaining about it. :smile:

stevenh commented 9 years ago

I'd keep that change separate if we did it. On 19/11/2014 07:42, Thomas Debesse wrote:

Hi, what do you think about renaming |tee.c| to |teeworlds.c| and qstat args to |-teeworldss| and |-teeworldsm| ? I see that newer games use complete name. Since the Teeworlds server support was broken since many years and the Teeworlds master support was never written before, there will not be many people complaining about it. :smile:

— Reply to this email directly or view it on GitHub https://github.com/multiplay/qstat/pull/4#issuecomment-63603020.

illwieckz commented 9 years ago

OK, can you confirme it is safe to free this strdup : https://github.com/illwieckz/qstat/commit/59661bac0b83859a80c5144cb54b558790cc4e2e#diff-ba6f07c5675bc4ade962db2a9e368d8eR147 ?

illwieckz commented 9 years ago

Hi, we are close to do it. I think the only one thing that prevent you to merge now is my question above, if you say yes, I will modify these lines and you will be able to merge after that.

illwieckz commented 9 years ago

Hi, I've updated.

The part with the useless strdup was rewritten, and I added an extra test to verify that the whole packet had a NULL end in it. In fact, all strnlen called on packet parts are called with a safe maxlength, so this extra test is probably not necessary.

stevenh commented 9 years ago

Your strnlen tests where fine but add_rule uses strdup so if its not null terminated then "boom" ;-)

stevenh commented 9 years ago

Any update on this @illwieckz ?

illwieckz commented 9 years ago

Hi @stevenh, I do not forget this, it's just that I do not have much time these days. I'll work on it soon. :wink:

stevenh commented 9 years ago

No problem just checking :)

illwieckz commented 9 years ago

Just to say, I will probably be able to work on it in july or august. I’m not forgetting this, I’m just very busy. :-)

illwieckz commented 9 years ago

Hi @stevenh, I modified my patch, if you want to test it you can do that:

qstat -default tees -f /tmp/teelist.txt

With this list as /tmp/teelist.txt.

vorot93 commented 9 years ago

This PR is nearly 1 year old. Outline any remaining issues in PR's description perhaps?