Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reserve protobuf tag for libv2ray #480

Closed
wants to merge 1 commit into from

Conversation

xiaokangwang
Copy link
Contributor

Reserve tag for libv2ray, so that libv2ray and v2ray-core can share the same configure file.

@xiaokangwang xiaokangwang self-assigned this Jul 18, 2017
@codecov
Copy link

codecov bot commented Jul 18, 2017

Codecov Report

Merging #480 into master will increase coverage by 0.29%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #480      +/-   ##
==========================================
+ Coverage   71.76%   72.05%   +0.29%     
==========================================
  Files         187      187              
  Lines        8528     8528              
==========================================
+ Hits         6120     6145      +25     
+ Misses       1857     1839      -18     
+ Partials      551      544       -7
Impacted Files Coverage Δ
proxy/http/server.go 56.47% <0%> (-1.18%) ⬇️
transport/internet/kcp/connection.go 74.78% <0%> (+0.58%) ⬆️
app/proxyman/mux/mux.go 69.49% <0%> (+0.84%) ⬆️
proxy/shadowsocks/client.go 74.46% <0%> (+2.12%) ⬆️
proxy/vmess/outbound/outbound.go 79.54% <0%> (+2.27%) ⬆️
proxy/socks/client.go 82.6% <0%> (+2.89%) ⬆️
proxy/socks/server.go 81.3% <0%> (+3.73%) ⬆️
proxy/shadowsocks/server.go 64.34% <0%> (+4.34%) ⬆️
common/buf/copy.go 68.75% <0%> (+8.33%) ⬆️
transport/internet/websocket/connection.go 60% <0%> (+8.88%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update eff193a...e6f8f1c. Read the comment docs.

@DarienRaymond
Copy link
Contributor

没有理解你的意思,你是打算在 libv2ray 里重写一遍这个 protobuf,然后把 12 那一项填上去么?

@xiaokangwang
Copy link
Contributor Author

我想的是protobuf会忽略自己不认识的项。因此可以通过两个项目都将自己需要的TAG事先声明好,将其他项目需要的项作为未知项忽略,通过仅定义和处理自己认识的项完成配置的读取。

具体来说libv2ray里面的定义差不多会是这样的:

message V2RaySharedConf{
reserved 1 to 5;
message LibV2RayConf{
省略
}
LibV2RayConf SharpLibV2Ray = 12;
}

优点:
手机客户端和电脑内核的pb配置文件是同一个,保证了同一个配置文件可以 直接复制粘贴 就可以在电脑上和手机上用。避免了需要多个配置文件或不兼容的配置文件的问题。特别是在pb文件是二进制格式,用户很难直接分辨出某个文件的格式的情况下,减少用户尝试次数和在处理配置文件的时候的复杂度。(毕竟现在本来就很少有用pb格式的配置文件的人)

程序在读取的时候会将自己不认识的项直接忽律。

在生成一个在电脑上和手机上都通用的pb配置文件时,不需要像json那样的复杂的复制粘贴(也是经常出错的一步)。可以直接将电脑配置文件部分和手机配置文件部分 用cat连接 (如果配置文件的内容是合理的话)。减少了在处理配置文件时需要的复杂的json括号匹配的问题(用户痛点之一)。

@DarienRaymond
Copy link
Contributor

proto3 不推荐类似这样的扩展方式(如 proto2 中的 extension 选项),并且 protobuf 也不推荐同时维护两个版本的 proto 定义。

可行的方法有以下两个:

一是 libv2ray 使用一个包含 core config 的 proto,比如

{
  LibV2RayConf libv2rayconf = 1;
  core.Config coreconf = 2;
}

二是在 core config 中模拟一个 extension,比如:

{
  repeated TypedMessage extension = 6;
}

然后把 libv2rayconf 以一个 TypedMessage 的形式写在 config 里。

至于你说的 cat 的方式,这属于 protobuf 的实现细节,我觉得会有风险。

@xiaokangwang
Copy link
Contributor Author

xiaokangwang commented Jul 20, 2017

首先是cat的问题,这个尽管是实现细节的一部分,但是cat两个pb文件的操作是有文档说明的。

The effect of these rules is that parsing the concatenation of two encoded messages produces exactly the same result as if you had parsed the two messages separately and merged the resulting objects.

@xiaokangwang
Copy link
Contributor Author

然后首先是你建议的第一个版本也是我最开始设计的时候的方案,存在一下问题:

  • v2ray-core,libv2ray会需要互相引用proto文件,导致不必要的项目之间的相互依赖关系。
  • v2ray-core不容易简单的识别配置文件是属于的类型,是仅包含了v2ray-core的配置还是包含了v2ray-core和libv2ray的配置(确定pb文件对应的message类型)。导致可能需要用户输入额外的信息才能确定pb的实际类型。导致用户混淆。
  • 不能使用cat连接合并pb文件。导致用户需要额外工具才能控制配置文件。

之后是第二个版本

  • 不能使用cat连接合并pb文件。导致用户需要额外工具才能控制配置文件。

我自己这里考虑的是现在的libv2ray,还有未来的插件都采用我这样的设计,之后在配置的时候可以有分别的配置文件格式(libv2ray将添加对于新的配置文件格式的支持),然后可以通过简单的cat聚合。提高对于不同场合的支持度。

@xiaokangwang
Copy link
Contributor Author

同时,我设计的pb文件的文件结构保持了和json文件的结构的一致性,防止了概念的混淆

@xiaokangwang
Copy link
Contributor Author

我的方法可能不完全符合pb的设计思路,但是却和V2Ray的需要结合的比较紧密。

@xiaokangwang
Copy link
Contributor Author

如果你坚持不接受我的提议的话,我比较推荐使用第二个方案。

@DarienRaymond
Copy link
Contributor

discussed offline. Closing this for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants