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

George/permissions #170

Merged
merged 8 commits into from
Jan 6, 2017
Merged

George/permissions #170

merged 8 commits into from
Jan 6, 2017

Conversation

palladia
Copy link
Contributor

@palladia palladia commented Jan 5, 2017

No description provided.

@palladia
Copy link
Contributor Author

palladia commented Jan 5, 2017

@paulcallen, @Microsoft/omi-devs


if [ "x$username" != "x" ]; then
userhome=`eval echo ~$username`
if [ -w $userhome ]; then
Copy link
Contributor

Choose a reason for hiding this comment

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

The -w flag only checks DAC, not for read-only file system. Do you want to catch an error down at 142, or is this too unlikely? If too unlikely, then why bother checking for -w here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Re-wrote function to:

createlink()
{
username=$OMI_USER

First, check if $OMI_USER's home directory is writable

if [ "x$username" != "x" ]; then
    userhome=`eval echo ~$username`
    if [ -w $userhome ]; then
        new_output="$userhome/$USER-$current_output"
        rm -rf $new_output
        mkdir $new_output
        if [ $? -eq 0 ]; then
            ln -s $new_output $current_output
        fi
    fi
fi

If that's not available, try /tmp

new_output_tmp="/tmp/$USER-$current_output"
rm -rf $new_output
mkdir $new_output
if [ $? -eq 0 ]; then
    ln -s $new_output $current_output
else
    echo "Unable to create softlink to $current_output.  Skipping..."
fi

}

@@ -228,7 +253,7 @@ cd omi-$version
## Create the prefix directory:
##

prefix=/tmp/OMI932E75578CAB46E4A3EB87787B9EA40F
prefix=/tmp/OMI932E75578CAB46E4A3EB87787B9EA40F-$USER
Copy link
Contributor

Choose a reason for hiding this comment

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

Okay, I'll bite. What the heck is 932E75578CAB46E4A3EB87787B9EA40F? Perhaps a comment is in order here ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No clue on why the long name was chosen.

Copy link
Member

Choose a reason for hiding this comment

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

I laughed when I saw this!

Copy link
Contributor

Choose a reason for hiding this comment

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

If there's no reason for this name, then it's needlessly confusing. Just pick a reasonable name, like /tmp/$USER-omi or, if that conflicts already, then /tmp/$USER-omi2 or whatever.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Renamed it to /tmp/omi2-$USER

@@ -537,7 +537,7 @@ static int StopServerSudo()

cleanup:
// To allow pid file to be deleted
Sleep_Milliseconds(200);
Sleep_Milliseconds(500);
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this work on all systems (even slow ones, like HP)? Perhaps better to have a tighter loop with a long wait time, and drop out if/when the PID file disappears?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've run this repeated on HP, and it seems to work. The increase in sleep here is really to give it more time before starting next test. Sometimes the next test starts up so fast that the "delete pid" operation deletes next test's omiserver pid file.

Copy link
Contributor

Choose a reason for hiding this comment

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

My point is it's timing sensitive. That makes it inherently risky. I don't want tests that work 90% of the time or 95% of the time, I want tests that work 100% of the time. AND: I want tests that don't take longer than they need to. So perhaps check if the PID is gone every 100 milliseconds up to 10 times or something.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've created issue #172 to address this.

@@ -22,7 +22,7 @@
# include <windows.h>
# include <strsafe.h>
#else
# include "../../common/linux/sal.h"
# include "linux/sal.h"
Copy link
Member

Choose a reason for hiding this comment

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

I think I would prefer: #include <common/Linux/sal.h>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Gcc command has "-I../common", if I remember correctly. I don't mind <linux/sal.h>?

Copy link
Member

Choose a reason for hiding this comment

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

yeah, I know that, but I just like things being relative to the top level. I am ok with Linux.sal.h but with angle-brackets which is used for doing search where quotes mean relative to current directory in general

Copy link
Contributor

Choose a reason for hiding this comment

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

Seeing <linux/sal.h> makes me think "system include file". If this is a local include file, I prefer "linux/sal.h" or something like that.

Copy link
Member

Choose a reason for hiding this comment

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

but quotes are supposed to be relative directories where as brackets are search path.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Originally header files within <> are supposed to be system headers, and those in quotes "" are supposed to be local ones, but since compiler doesn't enforce this, people have used them interchangeably. In OMI, I've seen both <> and "" local header files.

Technically it's "common/Linux/sal.h", but both works.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've changed it to "common/Linux/sal.h".

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.

None yet

3 participants