Menu
▾
▴
Re: JIT_SUPPORT: Running JIT dump file conversion as a special user [2/2]
|
From: Daniel H. <dan...@li...> - 2008-04-11 12:42:09
|
Hi,
as discussed in IRC here is my modified patch to use a special user
account to convert the JIT dump files.
Please review and give comments.
Thanks in advance.
Kind regards,
Daniel
Maynard Johnson wrote:
> Maynard Johnson wrote:
>> Here is the second patch to introduce the usage of the special user
>> account 'opjit' to do the conversion of the JIT dump files.
>>
>> A check in the makefile is included to warn if the user account is not
>> created yet.
>> The idea was to only check if the special user account exists because
>> there are many system environments where users who would use Oprofile
>> are not allowed to create user account by themselves. Therefore the
>> build process should not create that user account automatically.
> OK, but IMHO, the 'make install' process should do so. If the user runs
> 'configure --prefix=<my-home-dir>' and then 'make install' as a normal
> user, they should get the warning at both configure time and install
> time. BUT . . . if root runs 'make install', then the opjit user/group
> should be created if necessary.
>
> I think that rpm installs are typically based on the 'make install' for
> a project. We want to have an oprofile rpm from a distro be fully
> functional right out of the box. If we don't create the necessary
> user/group accounts at install time, the only bread crumb a user will
> get when they try to profile a java app and it doesn't work is the
> message "opjitconv: User information for special user opjit cannot be
> found" in the oprofiled.log -- and that's only if they're smart enough
> to look there.
>
> See a few other comments below.
>
> Regards,
> -Maynard
>
>>
>> Kind regards,
>> Daniel
>>
>>
>>
>> diff -prauN -X ../workspaces/eclipse/excludelist.txt
>> oprofile_conv_extracted/configure.in
>> ../workspaces/eclipse/oprofile_work/configure.in
>> --- oprofile_conv_extracted/configure.in 2008-01-09
>> 10:54:42.000000000 +0100
>> +++ ../workspaces/eclipse/oprofile_work/configure.in 2008-02-26
>> 15:27:52.000000000 +0100
>> @@ -265,3 +265,19 @@ AX_COPY_IF_CHANGE(doc/xsl/catalog-1.xml,
>> if test -z "$QT_LIB"; then
>> echo "Warning: a working Qt not found; no GUI will be built"
>> fi
>> +
>> +if test "`id -un opjit 2>/dev/null`" = "opjit" && \
>> + test "`id -gn opjit 2>/dev/null`" = "opjit"; then
>> + # nothing has to be done if user already exists
>> + :
>> +elif test `id -u` != "0"; then
>> + echo "Warning: The user 'opjit:opjit' cannot be found on your
>> systems user list."
>> + echo " Please ask your system administrator to add the
>> following user:"
>> + echo " user name : 'opjit'"
>> + echo " group name: 'opjit'"
>> +else
>> + echo "Warning: The user 'opjit:opjit' cannot be found on your
>> systems user list."
>> + echo " Please add the following user:"
>> + echo " user name : 'opjit'"
>> + echo " group name: 'opjit'"
>> +fi
>> diff -prauN -X ../workspaces/eclipse/excludelist.txt
>> oprofile_conv_extracted/doc/oprofile.xml
>> ../workspaces/eclipse/oprofile_work/doc/oprofile.xml
>> --- oprofile_conv_extracted/doc/oprofile.xml 2008-02-11
>> 10:35:35.000000000 +0100
>> +++ ../workspaces/eclipse/oprofile_work/doc/oprofile.xml 2008-02-26
>> 15:33:56.000000000 +0100
>> @@ -165,6 +165,14 @@ For information on how to use OProfile's
>> </para></listitem>
>> </varlistentry>
>> <varlistentry>
>> + <term>Required user account</term>
>> + <listitem><para>
>> + For secure processing sampled data (i.e. for Java JIT
>> code) the user opjit
>> + (group opjit) has to exist on the system. Otherwise
>> during configuration of
>> + the build process a warning will be shown to add that
>> special user account.
>> + </para></listitem>
>> + </varlistentry>
>> + <varlistentry>
>> <term>OProfile GUI</term>
>> <listitem><para>
>> The use of the GUI to start the profiler requires the
>> <filename>Qt 2</filename> library. <filename>Qt 3</filename> should
>> diff -prauN -X ../workspaces/eclipse/excludelist.txt
>> oprofile_conv_extracted/opjitconv/opjitconv.c
>> ../workspaces/eclipse/oprofile_work/opjitconv/opjitconv.c
>> --- oprofile_conv_extracted/opjitconv/opjitconv.c 2008-04-03
>> 11:04:02.000000000 +0200
>> +++ ../workspaces/eclipse/oprofile_work/opjitconv/opjitconv.c
>> 2008-04-07 18:33:10.000000000 +0200
>> @@ -24,6 +24,7 @@
>> #include <errno.h>
>> #include <fcntl.h>
>> #include <limits.h>
>> +#include <pwd.h>
>> #include <stdint.h>
>> #include <stdio.h>
>> #include <stdlib.h>
>> @@ -33,6 +34,9 @@
>> #include <unistd.h>
>> #include <wait.h>
>>
>> +#define SYSTEM_CMD_CP "/bin/cp"
>> +#define SYSTEM_CMD_RM "/bin/rm"
>> +
>> /*
>> * list head. The linked list is used during parsing (parse_all) to
>> * hold all jitentry elements. After parsing, the program works on the
>> @@ -50,6 +54,17 @@ enum bfd_architecture dump_bfd_arch;
>> int dump_bfd_mach;
>> char const * dump_bfd_target_name;
>>
> Is there a reason for all of these to be global variables?
>> +/* temporary working directory for dump file conversion step */
>> +char * tmp_conv_dir;
>> +/* copy of dump file created for conversion step */
>> +char * tmp_dumpfile;
>> +/* temporary ELF file created during conversion step */
>> +char * tmp_elffile;
>> +/* user information for special user 'opjit' */
>> +struct passwd * pw_opjit;
>> +
>> +char sys_cmd_buffer[PATH_MAX + 1];
>> +
>> /* the bfd handle of the ELF file we write */
>> bfd * cur_bfd;
>>
>> @@ -150,6 +165,88 @@ static char const * find_anon_dir_match(
>> return NULL;
>> }
>>
>> +int change_owner(char * path)
>> +{
>> + int rc = OP_JIT_CONV_OK;
>> + int fd;
>> + + fd = open(path, 0);
>> + if (fd < 0) {
>> + printf("opjitconv: File cannot be opened for changing
>> ownership.\n");
>> + rc = OP_JIT_CONV_FAIL;
>> + goto out;
>> + }
>> + if (fchown(fd, pw_opjit->pw_uid, pw_opjit->pw_gid) != 0) {
>> + printf("opjitconv: Changing ownership failed (%s).\n",
>> ((errno == EBADF) ? "EBADF" : ((errno == EINVAL) ? "EINVAL" : ((errno
>> == EPERM) ? "EPERM" : ((errno == EROFS) ? "EROFS" : "???")))));
> why not use strerror() or perror() here?
>
>> + close(fd);
>> + rc = OP_JIT_CONV_FAIL;
>> + goto out;
>> + }
>> + close(fd);
>> +
>> +out:
>> + return rc;
>> +}
>> +
>> +/* Copies the given file to the temporary working directory and sets
>> ownership
>> + * to 'opjit:opjit'.
>> + */
>> +int copy_dumpfile(char const * dumpfile)
>> +{
>> + int rc = OP_JIT_CONV_OK;
>> +
>> + sprintf(sys_cmd_buffer, "%s -p %s %s", SYSTEM_CMD_CP, dumpfile,
>> tmp_dumpfile);
>> +
>> + if (system(sys_cmd_buffer) != 0) {
>> + printf("opjitconv: Calling system() to copy files failed.\n");
>> + rc = OP_JIT_CONV_FAIL;
>> + goto out;
>> + }
>> +
>> + if (change_owner(tmp_dumpfile) != 0) {
>> + printf("opjitconv: Changing ownership of temporary dump file
>> failed.\n");
>> + rc = OP_JIT_CONV_FAIL;
>> + goto out;
>> + }
>> + +out:
>> + return rc;
>> +}
>> +
>> +/* Copies the created ELF file located in the temporary working
>> directory to the
>> + * final destination (i.e. given ELF file name) and sets ownership to
>> the
>> + * current user.
>> + */
>> +int copy_elffile(char * elf_file)
>> +{
>> + int rc = OP_JIT_CONV_OK;
>> + int fd;
>> +
>> + sprintf(sys_cmd_buffer, "%s -p %s %s", SYSTEM_CMD_CP,
>> tmp_elffile, elf_file);
>> + if (system(sys_cmd_buffer) != 0) {
>> + printf("opjitconv: Calling system() to copy files failed.\n");
>> + rc = OP_JIT_CONV_FAIL;
>> + goto out;
>> + }
>> +
>> + fd = open(elf_file, 0);
>> + if (fd < 0) {
>> + printf("opjitconv: File cannot be opened for changing
>> ownership.\n");
>> + rc = OP_JIT_CONV_FAIL;
>> + goto out;
>> + }
>> + if (fchown(fd, getuid(), getgid()) != 0) {
>> + printf("opjitconv: Changing ownership failed (%s).\n",
>> ((errno == EBADF) ? "EBADF" : ((errno == EINVAL) ? "EINVAL" : ((errno
>> == EPERM) ? "EPERM" : ((errno == EROFS) ? "EROFS" : "???")))));
> Same comment as above.
>> + close(fd);
>> + rc = OP_JIT_CONV_FAIL;
>> + goto out;
>> + }
>> + close(fd);
> [snip]
>
|