CABMAN: Remove _WIN32 ifdef's by dfbag7 · Pull Request #252 · reactos/reactos

dfbag7 · 2018-01-01T15:35:02Z

CORE-12645

Purpose

Proposed changes

Basically I tried to remove as much as possible _WIN32 ifdef's to make the code easier to maintain. To achieve that, I did the following changes:

Created two implementations of CCFDATAStorage class and placed them in CCFDATAStorageUnix.cxx and CCFDATAStorageWindows.cxx. Both implementations use the same definition of the class in cabinet.h header file.
Created single ifdef'ed section containing definitions of platform-dependent functions in the beginning of cabinet.h
Removed ifdef's in cabinet.cxx and dfp.cxx and make use the new platform-dependent functions defined in cabinet.h.

TODO

I tested the resulting code trying to create Hybrid CD in three environments:

RosBE for Windows using MSVC 2017 (compiler version 19.12.25830.2),
RosBE for Windows native (GCC v.4.7.2)
RowBe for Linux (Debian 8, GCC v.4.9.2)

Then I installed ReactOS from the resulting ISO image into VMWare VM. All worked fine.

There are, however, some ways to further polish the code. In particular, the code that chooses if existing target file should be overwritten when opening file for writing is still contains two ifdef'ed branches.

HBelusca · 2018-01-01T15:39:26Z

Hi, please squash your first two commits and fix your author/committer's name!

HBelusca · 2018-01-01T15:40:09Z

sdk/tools/cabman/CCFDATAStorageUnix.cxx

+/*
+* COPYRIGHT:   See COPYING in the top level directory
+* PROJECT:     ReactOS cabinet manager
+* FILE         tools/cabman/CCFDATAStorageWindows.cxx


The file name does not correspond to the actual one.

Thank you for your comments, I changed headers of the two files I've added.

GeoB99 · 2018-01-01T15:53:44Z

sdk/tools/cabman/CCFDATAStorageWindows.cxx

+ * PROGRAMMERS: 
+ * NOTES:
+ * REVISIONS:
+ */


The header file does not seem to meet the actual ReactOS Coding Style standard if I am not mistaken. NOTES and REVISIONS are unnecessary. Please refer to this article (File Structure) regarding this.

HeisSpiter · 2018-01-01T16:36:26Z

sdk/tools/cabman/CCFDATAStorageUnix.cxx

+* COPYRIGHT:   See COPYING in the top level directory
+* PROJECT:     ReactOS cabinet manager
+* FILE         tools/cabman/CCFDATAStorageWindows.cxx
+* PURPOSE:     CCFDATAStorage class implementation for Windows


Bad purpose too

HeisSpiter · 2018-01-01T16:37:40Z

sdk/tools/cabman/CCFDATAStorageUnix.cxx

+*/
+{
+    fclose(FileHandle);
+    FileHandle = tmpfile();


Why such a complex implementation?
Why not using ftruncate(fileno(FileHandle), 0); ?
And likely fseek(FileHandle, 0, SEEK_SET); beforehand.

Agree that this is not an optimal implementation, but current task is only to remove _WIN32, so I just left the implementation itself untouched.
Refactoring could be the next step.

OK for me then.

HeisSpiter · 2018-01-01T16:41:15Z

sdk/tools/cabman/cabinet.h

+    LONG size;
+    fseek(handle, 0, SEEK_END);
+    size = ftell(handle);
+    fseek(handle, 0, SEEK_SET);


I'm a bit skeptical about this... Wouldn't it be better to save current position, query for full size, and restore saved position?
So that, it would be consistent with Win32 behaviour.

Indeed, the call to fseek() in line 113 looks strange. But currently I'm trying to not mix tasks together, so I kept the implementation untouched only removing _WIN32 ifdef's.

ThFabba · 2018-01-01T18:09:54Z

sdk/tools/cabman/CCFDATAStorageUnix.cxx

+ *              Copyright 2017 Colin Finck <mail@colinfinck.de>
+ *              Copyright 2018 Dmitry Bagdanov <dimbo_job@mail.ru>   
+ */
+#if !defined(_WIN32)


So... this really isn't so much a Unix implementation as it is a standard C library one. Why don't we just delete the Win32 version and use this across the board? (I think that may have been the intent behind the Jira ticket)

I thought about that ;)
But then I decided to keep the initial intention to not use standard C library file access functions on Windows...

The idea was indeed to drop the win32 one,
since it looks like the other stuff was written after the win32 one.

Well that will be the next pull request then.

The problem with that approach is that what you propose now is not a natural step between the current version of the code and where we'd probably want it to be. E.g. changing if-CreateFile-else-fopen to fopen is much more straightforward than changing it to OpenFileForReading, and then back to the fopen that was already there before. That just seems like unnecessary code churn.
I'm open to discussion, but right now it seems to me like literally deleting all the stuff in the _WIN32 code path would get us closest to where we want to be.

ThFabba, now I'm completely agree with you. That was my misunderstanding that the intention is to not use standard C file access functions on Windows. So I'm going to provide another pull request that uses standard C functions on all platforms.

@dfbag7 For reference, if you push commits to the same branch they will end up in this pull request.
If you rewrite this branches history and then force-push you will update this pull request.

Otherwise, as the intent is totally different from original idea, perhaps should we close that PR for a brand new one? New code, new direction, new PR? :-)

Whatever the process here is - I'm fine with either updating or creating a new PR - I conclude this is WIP, so I'll add the WIP tag.

@dfbag7 Can you please make a basic performance evaluation after your changes? I am asking because I have experienced (mainly with STL) that portable code can lead to heavily degrading performance and this tool is already kinda slow, so I wouldn't want it to become even slower.

ColinFinck · 2018-01-11T16:36:15Z

@dfbag7 Thanks for working on this! This tool really needs some love.
While you're converting things to C++ classes/STL, you may also want to save some code by replacing my SEARCH_CRITERIA structure by an std::vector. I added that and some custom linked-list code years ago when I was still lacking STL proficiency..
Apart from that, I'm all for dropping the Win32-only code pathes and using STL for file access (alternatively stdio if that gives a better performance).

dfbag7 · 2018-01-12T01:26:22Z

Hi, I have done some testing and found that using native WIN32 functions for file access does not give any notable performance gain, so I'm going to remove all WIN32 native code in favor of stdio. Probably using STL will be the next step, thank you for the suggestion.

…

On 11.01.2018 23:36, Colin Finck wrote: @dfbag7 <https://github.com/dfbag7> Thanks for working on this! This tool really needs some love. While you're converting things to C++ classes/STL, you may also want to save some code by replacing my SEARCH_CRITERIA structure by an std::vector. I added that and some custom linked-list code years ago when I was still lacking STL proficiency.. Apart from that, I'm all for dropping the Win32-only code pathes and using STL for file access (alternatively stdio if that gives a better performance). — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#252 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AFndVKUKFz5bOQyzjs4L59WUmtvjVuavks5tJjiDgaJpZM4RQGXs>.

learn-more · 2018-01-12T09:12:55Z

In my experience moving fromg stdio to STL will incur a performance hit.

ThFabba · 2018-01-12T09:51:06Z

I'm guessing it's the processing, not the file I/O that makes cabman slow though?

dfbag7 · 2018-01-12T11:03:04Z

Yes. I have not done profiling, but almost sure that the I/O does not affect performance as much as zipping (which is actually performed by ZLIB library).

dfbag7 · 2018-01-23T08:16:08Z

Hi all,
I've added another commit. Can you please review? Now almost all file access and memory management calls to WIN32-specific functions are replaced with standard C library counterparts in both Linux and Windows environments.

learn-more

Hello @dfbag7, sorry for the late reply.
Overall this is already very good looking, I have added some inline comments about some details.

learn-more · 2018-01-25T09:17:39Z

sdk/tools/cabman/CCFDATAStorage.cxx

+CCFDATAStorage::CCFDATAStorage()
+/*
+* FUNCTION: Default constructor
+*/


If you want to use per-function comments, please stick to a format that is doxygen compatible, preferrably the one outlined here:
https://reactos.org/wiki/Coding_Style

The whole CABMAN project uses that style of function comments, so decided to not change them to maintain consistency.
Now the comments changed to the reactos' standard (only in file CCFDATAStorage.cxx file, though).
Commit fc99634

learn-more · 2018-01-25T09:17:55Z

sdk/tools/cabman/CCFDATAStorage.cxx

+        DPRINT(MID_TRACE, ("ERROR '%i'.\n", errno));
+        return CAB_STATUS_CANNOT_CREATE;
+    }
+    */


Leftover comments?

Those code was left from initial implementation, and I considered to not remove it...
Removed in commit 753efbe

learn-more · 2018-01-25T09:47:43Z

sdk/tools/cabman/CCFDATAStorage.cxx

+    }
+    */
+
+    FileCreated = true;


I think you can remove FileCreated alltogether, and just check on FileHandle != NULL ?

Removed in commit 753efbe

learn-more · 2018-01-25T09:48:54Z

sdk/tools/cabman/cabinet.h

-#define DIR_SEPARATOR_CHAR '\\'
-#define DIR_SEPARATOR_STRING "\\"
+    #define DIR_SEPARATOR_CHAR '\\'
+    #define DIR_SEPARATOR_STRING "\\"


Why is this indented?

Indentation removed in commit 299cede

For what it's worth, we seem to have three indentation styles for preprocessor directives in use throughout the codebase:

No indentation at all

Indenting like @dfbag7 did

Indenting the directive but not the hash (e.g. "# define")

I personally prefer the style @dfbag7 used. As our Coding Style guidelines give no rule regarding that, I use it myself in some modules.
However, it's up to you guys what shall be used for cabman in the future.

HeisSpiter

This new iteration looks good to me, I approve a squash & merge.
This will open room for improvement in the implementation :-).

ThFabba · 2018-01-25T18:08:18Z

Things look good on a cursory glance, but please make this at least 2 commits when merging. Moving existing code to a different file should always be its own commit, then changes to that code should be a second one.

dfbag7 · 2018-01-26T17:52:00Z

please make this at least 2 commits when merging.

done in that way - commits 2cd3c7b and d45a562

ThFabba · 2018-01-27T02:08:31Z

Perfect, thank you!

HBelusca reviewed Jan 1, 2018

View reviewed changes

GeoB99 reviewed Jan 1, 2018

View reviewed changes

learn-more changed the title ~~Core 12645~~ CABMAN: Remove _WIN32 ifdef's Jan 1, 2018

HeisSpiter requested changes Jan 1, 2018

View reviewed changes

dfbag7 force-pushed the CORE-12645 branch from f31e14f to 1c9e7d6 Compare January 1, 2018 17:35

ThFabba reviewed Jan 1, 2018

View reviewed changes

HeisSpiter approved these changes Jan 1, 2018

View reviewed changes

tkreuzer added the WIP label Jan 6, 2018

dfbag7 force-pushed the CORE-12645 branch from 7ae8c94 to 6af5a2a Compare January 21, 2018 16:22

learn-more reviewed Jan 25, 2018

View reviewed changes

HeisSpiter approved these changes Jan 25, 2018

View reviewed changes

dfbag7 added 2 commits January 26, 2018 22:44

[CABMAN] Remove _WIN32 ifdef's

2cd3c7b

[CABMAN] Move CCFDATAStorage class to a separate file

d45a562

dfbag7 force-pushed the CORE-12645 branch from fc99634 to d45a562 Compare January 26, 2018 17:35

ThFabba approved these changes Jan 27, 2018

View reviewed changes

HeisSpiter merged commit 991d33c into reactos:master Jan 27, 2018

sanchaez removed the WIP label Jan 27, 2018

dfbag7 deleted the CORE-12645 branch January 27, 2018 17:23

Uh oh!

Conversation

dfbag7 commented Jan 1, 2018

Purpose

Proposed changes

TODO

Uh oh!

HBelusca commented Jan 1, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dfbag7 Jan 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

learn-more Jan 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ColinFinck commented Jan 11, 2018

Uh oh!

dfbag7 commented Jan 12, 2018 via email

Uh oh!

learn-more commented Jan 12, 2018

Uh oh!

ThFabba commented Jan 12, 2018

Uh oh!

dfbag7 commented Jan 12, 2018

Uh oh!

dfbag7 commented Jan 23, 2018

Uh oh!

learn-more left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dfbag7 Jan 1, 2018 •

edited

Loading

learn-more Jan 1, 2018 •

edited

Loading