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

Fix compiler warnings (Visual Studio 2019) #232

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

imtiazdc
Copy link
Contributor

Problem: The Visual Studio 2019 compiler reports a lot of warnings in the ZFSin project. These changes are made to address some of those warnings which are genuine issues. NOTE: This change does not address all warnings in the project but a significant portion of them.

Cause: There are diverse reasons for the warnings. For example, not using proper datatypes that adapt to the platforms (x64 vs x86) for pointers. Not converting GUID to a string (proper format) before printing, etc.

Fix: Address each warning depending on why the compiler is complaining.

read_disk_info(int fd, diskaddr_t *capacity, uint_t *lbsize)
read_disk_info(uint64_t fd, diskaddr_t *capacity, uint_t *lbsize)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one is tricky. libefi is a library we got from ZOL, and due to that, it uses posix "int fd" for file-descriptors. I wrote the hacky "wosix" wrapper around that HANDLEs will "often" fit inside an int, and we convert (truncate) between the two using HTOI() and ITOH() - which should "hide" any warnings wrt to truncation.
We could claim ownership of libefi, and make it "ours", but much of libzfs also uses int fd, and changing that could make future merges painful. I can think of a few way to address this though.

Which leads to a bigger discussion on how we should handle that in future.

@@ -809,7 +809,7 @@ static int pthread_create(pthread_t *th, pthread_attr_t *attr, void *(* func)(vo
if (attr)
{
tv->p_state = attr->p_state;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. Bit of a personal style, even though "unsigned ssize;" is legal, I'm not a fan of it. Should it accidentally in the future be corrected to "unsigned int ssize;" I would not be objecting :)

@@ -45,7 +45,7 @@ extern int wosix_close(int fd);
extern int wosix_ioctl(int fd, unsigned long request, void *zc);
extern int wosix_read(int fd, void *data, uint32_t len);
extern int wosix_write(int fd, const void *data, uint32_t len);
extern int wosix_isatty(int fd);
extern int wosix_isatty(intptr_t fd);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which would defeat the purpose of a posix wrapper library taking posix "int fd" and handling it for the local platform.

man isatty
     int
     isatty(int fd);

@@ -240,7 +240,7 @@ dir_is_empty_stat(const char *dirname)
* We only want to return false if the given path is a non empty
* directory, all other errors are handled elsewhere.
*/
if (stat(dirname, &st) < 0 || !S_ISDIR(st.st_mode)) {
if (stat(dirname, (struct stat*)&st) < 0 || !S_ISDIR(st.st_mode)) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure we should define _stat64 then typecast it, probably better to change the type to fit the call.

@@ -7372,7 +7372,7 @@ zfsdev_open(dev_t dev, int flags, int devtype, struct proc *p)
#ifdef _WIN32
int flags = 0;
int devtype = 0;
struct proc *p = current_proc();
PEPROCESS p = current_proc();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is fine. But ultimately we want struct proc to be PEPROCESS - but I think perhaps the cleaner option is if we change upstream to use a bunch of porting typedefs, like a zfs_proc_t. I'll ask and see how receptive they are to that idea.

@lundman
Copy link
Collaborator

lundman commented Apr 1, 2020

I had a conversation over on slacker, with Ahrens, and got a tentative green light to make a zfs_fd_t type; defined something like

#ifdef _WIN32
typedef HANDLE zfs_fd_t;
#else 
typedef int zfs_fd_t;
#endif 

Name undecided.

Which means we can add that somewhere deep (spl includes) and update the zfs sources to use that instead of int fd everywhere. The wosix header hack part can be retired.

But possibly we might not want to do that right now, if the plans are to change to fit with openzfs v2 structure when that repo is ready.

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