Skip to content
This repository has been archived by the owner on Dec 27, 2019. It is now read-only.

tools: Rename IPC functions to reduce misleading #5

Closed
wants to merge 2 commits into from

Conversation

liuqun
Copy link

@liuqun liuqun commented Feb 21, 2019

Hi dear developers of WireGuard!

Recently, I've been reading the source code of WireGuard. The function name like ipc_get_device() looks to be a little bit misleading. To make the code more readable for new developers, here are some suggestions:

  • Rename ipc_get_device(&dev) to ipc_fetch_conf(&conf) because it only fetches the runtime configuations rather than a device instance or device handler.
  • Funtion prototype ipc_set_device(struct wgdevice *dev) is changed to ipc_set_device(const struct wgdevice *newconf).

Change the following IPC function names and parameters which used to be confusing:

- ipc_set_device(dev)                  => ipc_set_device(newconf)
- ipc_get_device(dev, interface)       => ipc_fetch_conf(conf, interface)
- kernel_set_device(dev)               => kernel_set_device(newconf)
- kernel_get_device(device, interface) => kernel_fetch_conf(conf, interface)
- userspace_set_device(dev)            => userspace_set_device(newconf)
- userspace_get_device(out, interface) => userspace_fetch_conf(out, interface)

Renamed local variable/parameter symbols:

  • In showconf_main(), show_main():
    device => conf
  • In ipc_fetch_conf(), kernel_fetch_conf(), userspace_fetch_conf():
    dev => conf
  • In ipc_set_device(), kernel_set_device(), userspace_set_device():
    dev => newconf

Note:
A lot of braces are added to the if-else code blocks to keep ourselves from making unexpected mistakes in the future.

Changed function prototypes:

  • int ipc_set_device(struct wgdevice *) => int ipc_set_device(const struct wgdevice *newconf)
  • int kernel_set_device(struct wgdevice *) => int kernel_set_device(const struct wgdevice *newconf)
  • int userspace_set_device(struct wgdevice *) => int userspace_set_device(const struct wgdevice *newconf)

@liuqun
Copy link
Author

liuqun commented Feb 21, 2019

@liuqun liuqun force-pushed the tools_refactor_ipc_func_name branch 6 times, most recently from 85812c5 to b869979 Compare February 22, 2019 03:41
@zx2c4-bot zx2c4-bot force-pushed the master branch 2 times, most recently from 93fe674 to 20a230b Compare February 26, 2019 22:01
@liuqun liuqun force-pushed the tools_refactor_ipc_func_name branch from b869979 to 3cc6c6e Compare February 28, 2019 01:58
liuqun added a commit to liuqun/WireGuard that referenced this pull request Feb 28, 2019
Improve the coding style of IPC APIs for WireGuard tools.
This commit follows a previous PR at WireGuard#5

Signed-off-by: 刘群 <[email protected]>
The function name like ipc_get_device() is a little bit misleading.
To make the code more readable for new developers, here are some suggestions:
* Rename ipc_get_device(&dev) to ipc_fetch_conf(&conf) because it only fetches the runtime configuations from kernel.
* Funtion prototype ipc_set_device(struct wgdevice *dev) is changed to ipc_set_device(const struct wgdevice *newconf).

Change the following IPC function names and parameters which used to be confusing:
ipc_set_device(dev)                  => ipc_set_device(newconf)
ipc_get_device(dev, interface)       => ipc_fetch_conf(conf, interface)
kernel_set_device(dev)               => kernel_set_device(newconf)
kernel_get_device(device, interface) => kernel_fetch_conf(conf, interface)
userspace_set_device(dev)            => userspace_set_device(newconf)
userspace_get_device(out, interface) => userspace_fetch_conf(out, interface)

Renamed local variable/parameter symbols:
* In showconf_main(), show_main():
  device => conf
* In ipc_fetch_conf(), kernel_fetch_conf(), userspace_fetch_conf():
  dev => conf
* In ipc_set_device(), kernel_set_device(), userspace_set_device():
  dev => newconf

Note:
A lot of braces are added to the if-else code blocks to keep ourselves from making unexpected mistakes in the future.

Changed function prototypes:
int ipc_set_device(struct wgdevice *) => int ipc_set_device(const struct wgdevice *newconf)
int kernel_set_device(struct wgdevice *) => int kernel_set_device(const struct wgdevice *newconf)
int userspace_set_device(struct wgdevice *) => int userspace_set_device(const struct wgdevice *newconf)

Signed-off-by: Liu Qun <[email protected]>
@liuqun liuqun force-pushed the tools_refactor_ipc_func_name branch 2 times, most recently from 7f22bcc to 2ad1122 Compare February 28, 2019 03:16
Improve the coding style of IPC APIs for WireGuard tools.

Signed-off-by: Liu Qun <[email protected]>
@liuqun liuqun force-pushed the tools_refactor_ipc_func_name branch from 2ad1122 to c9059b8 Compare February 28, 2019 03:56
@thinrope
Copy link

While I like the ideas in this, they are a bit intrusive and change the coding style in a way. And do it in one single big patch.
I think it will be a bit more easy to digest if the "adding extra curly braces" part was a separate diff.
Also, while at it, using a standard C-beautifyer (astyle, uncrustify, etc.) for the whole project and selecting a coding style might be a better step.

@zx2c4-bot zx2c4-bot force-pushed the master branch 2 times, most recently from 4ffbc29 to 033dc89 Compare February 28, 2019 22:53
@liuqun
Copy link
Author

liuqun commented Mar 1, 2019

Thanks @thinrope !

Now I've removed those curly braces and created a new PR #6

@liuqun liuqun closed this Mar 1, 2019
@liuqun
Copy link
Author

liuqun commented Mar 1, 2019

I've found the on-line document Linux kernel coding style here from Intel's opensource web site 01.org:
https://01.org/linuxgraphics/gfx-docs/drm/process/coding-style.html#placing-braces-and-spaces

In the document:

Do not unnecessarily use braces where a single statement will do.

	if (condition)
		action();

and

	if (condition)
		do_this();
	else
		do_that();

This does not apply if only one branch of a conditional statement is a single
statement; in the latter case use braces in both branches:

	if (condition) {
		do_this();
		do_that();
	} else {
		otherwise();
	}

If we look into WireGuard/src/tools, we might found many if-else blocks didn‘t follow the last rule above, for example:
https://github.com/WireGuard/WireGuard/blob/033dc8974de052c1f1ab5d735ec2ee921127e352/src/tools/show.c#L447-L452

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

Successfully merging this pull request may close these issues.

2 participants