Finish porting advapi32 to v0.3 #447

austinwagner · 2017-05-21T23:53:05Z

Used @Susurrus's fork as a base.

All functions exported by advapi32 that can be found in the SDK headers* have been added to their corresponding Rust files with the exception of SetServiceBits (um/LMServer.h). If a Rust version of a header already existed then only the required pieces were added to the file. If the Rust file didn't exist already the full header was ported to the extent I was able to.

*139 functions are exported from the DLL but have no declaration in any header in the latest Windows SDK.

austinwagner · 2017-05-22T01:19:20Z

Unused macros? Is this a CI issue or did I break something? I tried building each of the features that I added or modified with Rust stable and I don't get any errors.

retep998 · 2017-05-22T01:26:00Z

It's a new nightly lint that got caught in deny(unused). However because 99.9% of the crate disables itself without any features enabled, the macros remain and end up unused.

Susurrus · 2017-05-22T01:28:03Z

@austinwagner That issue is being addressed in #446.

Susurrus · 2017-05-22T18:48:58Z

@austinwagner Your header import declarations in build.rs aren't listed alphabetically, which is why they're failing the tests.

austinwagner · 2017-05-22T18:56:48Z

Yes it's clear why it's failing this time. 🙂 Apparently I'm not good at quickly eyeballing alphabetical order. I'm going to fix it as soon as I get home from work. The error message shows me all of the build.rs sorting problems, but is there a way to quickly sort the imports in the other files? I haven't tried rustfmt because I don't know if it will keep the rest of the file formatted the way that our Benevolent Bunny likes.

Susurrus · 2017-05-22T19:54:29Z

I believe the only errors are in build.rs, so I don't think you need to reorder anything in any of the actual .rs files. But regarding rustfmt, #444 is tracking its suitability for winapi-rs.

austinwagner · 2017-05-25T22:34:10Z

Is using the memset and strnlen from ntdll acceptable? I know that anything exported by ntdll is considered to be an internal function, but I doubt they're going away. If it's a problem I can replace them with simple loop implementations though.

retep998 · 2017-05-26T05:53:20Z

core::ptr::write_bytes is a thing you know.

austinwagner · 2017-05-26T08:20:59Z

No I did not know. My search for intrinsics led me to a page that said they will likely never make it to stable. "...they should be used through stabilized interfaces in the rest of the standard library" makes it sound like you can't access the intrinsics on stable when no_std is set.

retep998 · 2017-05-26T08:50:43Z

core::ptr::write_bytes is stable though, despite being under the intrinsics.

Susurrus · 2017-06-01T22:17:50Z

@austinwagner Needs rebasing now that #448 was merged.

austinwagner · 2017-06-01T22:55:44Z

@Susurrus Is rebasing the right approach or should I just merge again like I did for smaller conflicts? I've never contributed to a large, popular repo so I don't know what's considered best practice.

Susurrus · 2017-06-01T22:56:56Z

I always rebase rather than merge as if bisecting ever needs to be done, it's easier with less merge nodes. I also find it easier to review PRs that don't have a ton of extraneous "merge ..." nodes.

Susurrus · 2017-06-01T22:58:30Z

I also rebase and squash/edit commits to get rid of all of those small "fix ..." commits (like you have in this commit). The way I look at my commits is that they should always result in a compilable source tree. That's not always possible, but I strive for it. So merging smaller fix commits into the original commit that broke thing before merging is nice.

austinwagner · 2017-06-01T23:07:11Z

You answered my next question before I got to ask it 😃 I'll take care of this as soon as I'm able so the PR can be nice and clean!

I do have one more question for now: was splitting up the commits by header a good approach? I specifically used hunk staging to isolate each one even though I didn't actually start making commits until I finished the porting.

retep998 · 2017-06-01T23:08:15Z

You have some commits which are actually rebased versions of commits by @Susurrus which makes it a lot harder to review this PR. Once those commits are rebased away so this PR is just your commits, I'll be able to review it.

Susurrus · 2017-06-01T23:09:25Z

@austinwagner Re the single-header commits, I haven't seen @retep998 complain about it yet, but it at least makes it easy for me to review and check that I have all the necessary changes for a single header/family of headers to be enabled.

austinwagner · 2017-06-03T01:13:32Z

All cleaned up with a rebase

Susurrus · 2017-06-03T03:43:40Z

timezoneapi's functions are in kernel32, so that needs to specified as a dependency. Additionally, those functions are all in lib/kernel32/src/lib.rs. Can you remove those functions from that file in that same commit?

Susurrus · 2017-06-03T03:46:26Z

src/shared/evntrace.rs

+    LoggerNameOffset: ULONG,
+}}
+pub type PEVENT_TRACE_PROPERTIES = *mut EVENT_TRACE_PROPERTIES;
+// TODO: The single item structs are defined with the ":" bitfield syntax


Does this TODO still need to be resolved?

Oh it looks like I simply forgot to remove the comment after I found the macro to handle bitfields.

Susurrus · 2017-06-03T03:56:32Z

src/shared/evntprov.rs

+}
+#[inline(always)]
+pub unsafe fn EventDescCreate(
+    EventDescriptor: PEVENT_DESCRIPTOR, Id: USHORT, Version: UCHAR, Channel: UCHAR, Level: UCHAR,


These arguments all need to be on different lines.

This applies to the following function declarations as well.

I didn't realize that style applied to function definitions. Thought it was only for the declarations. I'll go through all the files and validate that I don't have the same issue elsewhere.

@retep998 Looking at some existing #[inline] functions, if everything up to and including the open curly bracket fits within the 99 character limit then the parameters should all stay on a single line, correct?

For function definitions (functions with bodies), if the function signature fits within a single line then it can stay on one line but it's not required. If it isn't a single line, then it should follow the same one parameter per line that all other definitions follow.

However, you shouldn't be using #[inline(always)]. Function definitions should always use #[inline].

retep998 · 2017-06-03T15:27:27Z

src/shared/sddl.rs

+        StringSecurityDescriptor: *mut LPWSTR,
+        StringSecurityDescriptorLen: PULONG,
+    ) -> BOOL;
+}


Don't forget end of file newlines.

Does Github ignore EOF newlines? It was missing before, but it's definitely there now. Checked the file directly downloaded from Github in a hex editor to make sure I wasn't going crazy.

retep998 · 2017-06-03T15:29:36Z

src/shared/wmistr.rs

+#[cfg(target_arch = "x86_64")]
+const __SIZE_OF_WMIREGGUIDW_u: usize = 8;
+UNION2!{union WMIREGGUIDW_u {
+    [u8; __SIZE_OF_WMIREGGUIDW_u],


I recently added support for UNION2 to have two storages specified, one for 32bit and one for 64bit. Also, an array of u8 is usually the wrong thing because the union needs to have the correct alignment (otherwise when placed inside a struct it could have the wrong position resulting in issues like #452).

How do you determine which to use then? I just got the sizes by looking at the output of sizeof() in C++ for both 32 and 64 bit builds.

char const * type_for_alignment(uintptr_t align) { switch (align) { case 1: return "u8"; case 2: return "u16"; case 4: return "u32"; case 8: return "u64"; } throw; } #define PRINT_UNION(x) cout << "[" << type_for_alignment(alignof(x)) << "; " << sizeof(x) / alignof(x) << "]" << endl;

That's handy! I think it makes sense to add this block of code and the new UNION2 style to CONTRIBUTING.md. The rules regarding function definition styles and EOF newlines should probably also be there.

@austinwagner UNION2 is already documented. I just updated #456 about the other things discussed her excluding this example code. I'll add this alignment example to UNION2 however.

Sorry, you're right that the dual-alignment UNION2 isn't documented. I'm doing that now.

@austinwagner Does my latest commit for #456 make things clear?

Susurrus · 2017-06-07T19:08:49Z

@austinwagner This needs to be updated with the recent merge of #454.

Fixes: - Add namedpipeapi to Cargo.toml - Typo correction in namedpipeapi header

retep998 · 2018-04-02T15:31:26Z

PR currently failing CI.

* Mark timezoneapi as dependent on advapi32.dll * Add advapi32 functions to winsvc * Add accctrl in preparation for aclapi * Add aclapi * Add ImpersonateNamedPipeClient to namedpipeapi Fixes: - Add namedpipeapi to Cargo.toml - Typo correction in namedpipeapi header * Add advapi32 functions to lsalookup * Add winefs * Add advapi32 functions to wincred * Add wct * Add advapi32 functions to securitybaseapi * Add advapi32 functions to processthreadsapi * Add advapi32 functions to ntsecapi * Add evntprov * Add wmistr * Add evntrace and evntcons * Add sddl * Add appmgmt * Add ntlsa * Add mschapp * Add perflib * Add winsafer * Fix securitybaseapi imports

Susurrus mentioned this pull request May 22, 2017

Added GetSecurityInfo #315

Closed

austinwagner force-pushed the finish_advapi32 branch 2 times, most recently from 5108a82 to c5e9b05 Compare June 3, 2017 00:52

Susurrus reviewed Jun 3, 2017

View reviewed changes

retep998 reviewed Jun 3, 2017

View reviewed changes

Susurrus mentioned this pull request Jun 3, 2017

Add more code style requirements #456

Closed

austinwagner force-pushed the finish_advapi32 branch from c5e9b05 to c4f2cdb Compare June 3, 2017 20:26

austinwagner force-pushed the finish_advapi32 branch from c4f2cdb to 19ff1a0 Compare June 8, 2017 23:11

austinwagner added 21 commits January 1, 2018 22:11

Mark timezoneapi as dependent on advapi32.dll

2706d3c

Add advapi32 functions to winsvc

7f3a41d

Add accctrl in preparation for aclapi

18c8032

Add aclapi

ff637e6

Add ImpersonateNamedPipeClient to namedpipeapi

0f27ad8

Fixes: - Add namedpipeapi to Cargo.toml - Typo correction in namedpipeapi header

Add advapi32 functions to lsalookup

cfadff0

Add winefs

c4aae57

Add advapi32 functions to wincred

21dab32

Add wct

59f56c5

Add advapi32 functions to securitybaseapi

d9bb617

Add advapi32 functions to processthreadsapi

83e5383

Add advapi32 functions to ntsecapi

b64fd97

Add evntprov

0eb4b57

Add wmistr

a9eae77

Add evntrace and evntcons

93aa4d2

Add sddl

540e652

Add appmgmt

56c289b

Add ntlsa

f921417

Add mschapp

e9f7360

Add perflib

3195f78

Add winsafer

80ce6ee

austinwagner force-pushed the finish_advapi32 branch from 3139b95 to 80ce6ee Compare January 2, 2018 03:18

retep998 changed the base branch from dev to 0.3 February 4, 2018 05:46

Merge branch '0.3' into finish_advapi32

92591b1

Fix securitybaseapi imports

5ab9b7c

retep998 merged commit 4a69b4a into retep998:0.3 Apr 6, 2018

skdltmxn mentioned this pull request Apr 30, 2018

Add aclapi definitions #516

Closed

pronebird mentioned this pull request May 22, 2018

Add StartService declarations #608

Closed

Finish porting advapi32 to v0.3 #447

Finish porting advapi32 to v0.3 #447

Conversation

austinwagner commented May 21, 2017

austinwagner commented May 22, 2017

retep998 commented May 22, 2017 • edited Loading

Susurrus commented May 22, 2017

Susurrus commented May 22, 2017

austinwagner commented May 22, 2017

Susurrus commented May 22, 2017

austinwagner commented May 25, 2017

retep998 commented May 26, 2017

austinwagner commented May 26, 2017

retep998 commented May 26, 2017 • edited Loading

Susurrus commented Jun 1, 2017

austinwagner commented Jun 1, 2017

Susurrus commented Jun 1, 2017

Susurrus commented Jun 1, 2017

austinwagner commented Jun 1, 2017

retep998 commented Jun 1, 2017

Susurrus commented Jun 1, 2017

austinwagner commented Jun 3, 2017

Susurrus commented Jun 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Susurrus Jun 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Susurrus commented Jun 7, 2017

retep998 commented Apr 2, 2018

retep998 commented May 22, 2017 •

edited

Loading

retep998 commented May 26, 2017 •

edited

Loading

Susurrus Jun 3, 2017 •

edited

Loading