[PATCH] Add version option to newrole

[PATCH] Add version option to newrole

[Glauber de Oliveira Costa]

[-- Attachment #1: Type: text/plain, Size: 365 bytes --]

Here's a new version that incorporates your suggestions. I moved the constant 
definition to the makefile, and also, I'm gathering the data from the version 
file. Also, Steve, I would like to hear from you why do you think the -v 
option is not that descriptive.

If you guys accept this patch, my obvious next step would be adding it to the 
manpage. 

Glauber.


[-- Attachment #2: patch-newrole.v2 --]
[-- Type: text/x-diff, Size: 2610 bytes --]

diff -upNr policycoreutils-1.29.18/newrole/Makefile policycoreutils-1.29.18.mine/newrole/Makefile
--- policycoreutils-1.29.18/newrole/Makefile	2006-02-02 15:08:13.000000000 -0200
+++ policycoreutils-1.29.18.mine/newrole/Makefile	2006-02-08 14:25:41.000000000 -0200
@@ -10,9 +10,10 @@ AUDITH = $(shell ls /usr/include/libaudi
 # This is so that we have the CAP_AUDIT_WRITE capability. newrole will
 # shed all privileges and change to the user's uid.
 LOG_AUDIT_PRIV ?= n
+VERSION = $(shell cat ../VERSION)
 
 CFLAGS ?= -Werror -Wall -W
-override CFLAGS += $(LDFLAGS) -I$(PREFIX)/include -DUSE_NLS -DLOCALEDIR="\"$(LOCALEDIR)\"" -DPACKAGE="\"policycoreutils\""
+override CFLAGS += -DVERSION=\"$(VERSION)\" $(LDFLAGS) -I$(PREFIX)/include -DUSE_NLS -DLOCALEDIR="\"$(LOCALEDIR)\"" -DPACKAGE="\"policycoreutils\""
 LDLIBS += -lselinux -lsepol -L$(PREFIX)/lib
 ifeq (${PAMH}, /usr/include/security/pam_appl.h)
 	override CFLAGS += -DUSE_PAM
Binary files policycoreutils-1.29.18/newrole/newrole and policycoreutils-1.29.18.mine/newrole/newrole differ
diff -upNr policycoreutils-1.29.18/newrole/newrole.c policycoreutils-1.29.18.mine/newrole/newrole.c
--- policycoreutils-1.29.18/newrole/newrole.c	2006-02-02 15:08:13.000000000 -0200
+++ policycoreutils-1.29.18.mine/newrole/newrole.c	2006-02-08 14:26:19.000000000 -0200
@@ -10,7 +10,7 @@
  *
  * USAGE:
  *
- * newrole [ -r role ] [ -t type ] [ -l level ] [ -- args ]
+ * newrole [ -r role ] [ -t type ] [ -l level ] [ -v ] [ -- args ]
  *
  * BUILD OPTIONS:
  *
@@ -84,7 +84,7 @@
 #endif
 
 /* USAGE_STRING describes the command-line args of this program. */
-#define USAGE_STRING "USAGE: newrole [ -r role ] [ -t type ] [ -l level ] [ -- args ]"
+#define USAGE_STRING "USAGE: newrole [ -r role ] [ -t type ] [ -l level ] [ -v ] [ -- args ]"
 
 #define DEFAULT_CONTEXT_SIZE 255  /* first guess at context size */
 
@@ -417,6 +417,7 @@ int main( int argc, char *argv[] ) {
     { "role", 1, 0, 'r' },
     { "type", 1, 0, 't' },
     { "level", 1, 0, 'l' },
+    { "version", 0, 0, 'v' },
     { NULL, 0, 0, 0 }
   };
   char *role_s = NULL;               /* role spec'd by user in argv[] */
@@ -465,11 +466,15 @@ int main( int argc, char *argv[] ) {
   }
 
   while (1) {
-    clflag=getopt_long(argc,argv,"r:t:l:",long_options,&flag_index);
+    clflag=getopt_long(argc,argv,"r:t:l:v",long_options,&flag_index);
     if (clflag == -1)
 	break;
     
     switch( clflag ) {
+    case 'v':
+	printf("newrole version %s\n",VERSION);
+	exit(0);
+	break;
     case 'r':
       /* If role_s is already set, the user spec'd multiple roles - bad. */
       if( role_s ) {

Re: [PATCH] Add version option to newrole

[Stephen Smalley]

On Wed, 2006-02-08 at 14:45 -0200, Glauber de Oliveira Costa wrote:
> Here's a new version that incorporates your suggestions. I moved the constant 
> definition to the makefile, and also, I'm gathering the data from the version 
> file. Also, Steve, I would like to hear from you why do you think the -v 
> option is not that descriptive.
> 
> If you guys accept this patch, my obvious next step would be adding it to the 
> manpage. 

I think his complaint was with our existing usage message (the args
part), not your change.

-- 
Stephen Smalley
National Security Agency


--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [PATCH] Add version option to newrole

[Steve G]

>I moved the constant definition to the makefile, and also, I'm gathering the
data
>from the version file.

Thanks. That should be easier to maintain.

>Also, Steve, I would like to hear from you why do you think the -v
>option is not that descriptive.

OK my bad. I though the -- ARGS was added. -v is fine.

But waht about the other programs? load_policy, restorecon, run_init, etc...

-Steve

__________________________________________________
Do You Yahoo!?
Tired of spam?  Yahoo! Mail has the best spam protection around 
http://mail.yahoo.com 

--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [PATCH] Add version option to newrole

[Glauber de Oliveira Costa]

Em Quarta 08 Fevereiro 2006 15:10, Steve G escreveu:
> >I moved the constant definition to the makefile, and also, I'm gathering
> > the
>
> data
>
> >from the version file.
>
> Thanks. That should be easier to maintain.

Np. 

> >Also, Steve, I would like to hear from you why do you think the -v
> >option is not that descriptive.
>
> OK my bad. I though the -- ARGS was added. -v is fine.
>
> But waht about the other programs? load_policy, restorecon, run_init,
> etc...

Note that it would be too much spent work *IF* you don't like the idea. So, 
here is newrole implementation for your appreciation. If you guys tell me 
"Ok, version option is good, we'll merge it", I'll then implement it in other 
programs, and document it in their respective manpages.

Also, if there are any of the policycoreutils programs that for any reason 
should *not* have a version option for any reason, please let me know the 
program along with the reasons, and I'll skip it.

> -Steve

Glauber.

> __________________________________________________
> Do You Yahoo!?
> Tired of spam?  Yahoo! Mail has the best spam protection around
> http://mail.yahoo.com

--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [PATCH] Add version option to newrole

[Stephen Smalley]

On Wed, 2006-02-08 at 14:45 -0200, Glauber de Oliveira Costa wrote:
> Here's a new version that incorporates your suggestions. I moved the constant 
> definition to the makefile, and also, I'm gathering the data from the version 
> file. Also, Steve, I would like to hear from you why do you think the -v 
> option is not that descriptive.
> 
> If you guys accept this patch, my obvious next step would be adding it to the 
> manpage. 

At present, your patch causes newrole to display the version number of
the entire collection (policycoreutils), not an individual version tied
to the newrole program itself.  Thus, I think it should display the
PACKAGE string as well (already defined) as part of the message, e.g.:
	printf("%s:  %s version %s\n", argv[0], PACKAGE, VERSION);
yielding output like:
	newrole:  policycoreutils version 1.29.20

Otherwise, the patch seems sane and suitable for the other utilities as
well.  A few of the utilities appear to have their own version string at
present, but I think you can just drop that as no one is maintaining
that - only the top-level VERSION for the entire collection is
meaningful.

-- 
Stephen Smalley
National Security Agency


--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [PATCH] Add version option to newrole

[Stephen Smalley]

On Thu, 2006-02-09 at 15:09 -0500, Stephen Smalley wrote:
> At present, your patch causes newrole to display the version number of
> the entire collection (policycoreutils), not an individual version tied
> to the newrole program itself.  Thus, I think it should display the
> PACKAGE string as well (already defined) as part of the message, e.g.:
> 	printf("%s:  %s version %s\n", argv[0], PACKAGE, VERSION);
> yielding output like:
> 	newrole:  policycoreutils version 1.29.20
> 
> Otherwise, the patch seems sane and suitable for the other utilities as
> well.  A few of the utilities appear to have their own version string at
> present, but I think you can just drop that as no one is maintaining
> that - only the top-level VERSION for the entire collection is
> meaningful.

Also, using -v is ambiguous, as it often means to run a program under a
verbose mode (extra output), and several utilities are using -v in that
way, so use -V and/or --version instead.

-- 
Stephen Smalley
National Security Agency


--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [PATCH] Add version option to newrole

[Glauber de Oliveira Costa]

Em Quinta 09 Fevereiro 2006 18:12, Stephen Smalley escreveu:
> On Thu, 2006-02-09 at 15:09 -0500, Stephen Smalley wrote:
> > At present, your patch causes newrole to display the version number of
> > the entire collection (policycoreutils), not an individual version tied
> > to the newrole program itself.  Thus, I think it should display the
> > PACKAGE string as well (already defined) as part of the message, e.g.:
> > 	printf("%s:  %s version %s\n", argv[0], PACKAGE, VERSION);
> > yielding output like:
> > 	newrole:  policycoreutils version 1.29.20

argv[0] will display the complete path of the command being called. I think 
this is not what we want. 

> > Otherwise, the patch seems sane and suitable for the other utilities as
> > well.  A few of the utilities appear to have their own version string at
> > present, but I think you can just drop that as no one is maintaining
> > that - only the top-level VERSION for the entire collection is
> > meaningful.
>
> Also, using -v is ambiguous, as it often means to run a program under a
> verbose mode (extra output), and several utilities are using -v in that
> way, so use -V and/or --version instead.

Sure this is confusing. A lot of programs use -v for version (see GNU 
Binutils), some don't use a shortopt at all (see GNU coreutils), and some use 
the suggested -V (see GNU zip). I'll cope with your suggestion and use -V 

I'll send a new version of newrole's implementation together with the manpage 
patch soon, and patches for the other programs as soon as I can too.

glauber.
 

--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

[PATCH] add version option to newrole

[Glauber de Oliveira Costa]

[-- Attachment #1: Type: text/plain, Size: 412 bytes --]

When trying to isolate a problem today, I missed a lot an version option (-v 
or such). This simple patch adds such an option.

If it's of interest, such an option may be added to whole policycoreutils 
package. (IMHO, it would be useful). With this possibility in mind, I decided 
to  implement the version string in a separate version.h file, that could be 
included by other packages too. 

Patch follows.




[-- Attachment #2: patch-newrole --]
[-- Type: text/x-diff, Size: 2030 bytes --]

diff -upNr policycoreutils-1.29.18/newrole/newrole.c policycoreutils-1.29.18.mine/newrole/newrole.c
--- policycoreutils-1.29.18/newrole/newrole.c	2006-02-02 15:08:13.000000000 -0200
+++ policycoreutils-1.29.18.mine/newrole/newrole.c	2006-02-07 16:16:23.000000000 -0200
@@ -10,7 +10,7 @@
  *
  * USAGE:
  *
- * newrole [ -r role ] [ -t type ] [ -l level ] [ -- args ]
+ * newrole [ -r role ] [ -t type ] [ -l level ] [ -v ] [ -- args ]
  *
  * BUILD OPTIONS:
  *
@@ -82,9 +82,10 @@
 #ifndef PACKAGE
 #define PACKAGE "policycoreutils"   /* the name of this package lang translation */
 #endif
+#include "version.h"
 
 /* USAGE_STRING describes the command-line args of this program. */
-#define USAGE_STRING "USAGE: newrole [ -r role ] [ -t type ] [ -l level ] [ -- args ]"
+#define USAGE_STRING "USAGE: newrole [ -r role ] [ -t type ] [ -l level ] [ -v ] [ -- args ]"
 
 #define DEFAULT_CONTEXT_SIZE 255  /* first guess at context size */
 
@@ -417,6 +418,7 @@ int main( int argc, char *argv[] ) {
     { "role", 1, 0, 'r' },
     { "type", 1, 0, 't' },
     { "level", 1, 0, 'l' },
+    { "version", 0, 0, 'v' },
     { NULL, 0, 0, 0 }
   };
   char *role_s = NULL;               /* role spec'd by user in argv[] */
@@ -465,11 +467,15 @@ int main( int argc, char *argv[] ) {
   }
 
   while (1) {
-    clflag=getopt_long(argc,argv,"r:t:l:",long_options,&flag_index);
+    clflag=getopt_long(argc,argv,"r:t:l:v",long_options,&flag_index);
     if (clflag == -1)
 	break;
     
     switch( clflag ) {
+    case 'v':
+	printf("newrole version %s\n",VERSION);
+	exit(0);
+	break;
     case 'r':
       /* If role_s is already set, the user spec'd multiple roles - bad. */
       if( role_s ) {
diff -upNr policycoreutils-1.29.18/newrole/version.h policycoreutils-1.29.18.mine/newrole/version.h
--- policycoreutils-1.29.18/newrole/version.h	1969-12-31 21:00:00.000000000 -0300
+++ policycoreutils-1.29.18.mine/newrole/version.h	2006-02-07 16:15:47.000000000 -0200
@@ -0,0 +1,4 @@
+
+#ifndef VERSION
+#define VERSION "1.29.18"
+#endif

Re: [PATCH] add version option to newrole

[Stephen Smalley]

On Wed, 2006-02-08 at 09:34 -0200, Glauber de Oliveira Costa wrote:
> When trying to isolate a problem today, I missed a lot an version option (-v 
> or such). This simple patch adds such an option.
> 
> If it's of interest, such an option may be added to whole policycoreutils 
> package. (IMHO, it would be useful). With this possibility in mind, I decided 
> to  implement the version string in a separate version.h file, that could be 
> included by other packages too. 
> 
> Patch follows.

If we were going to do this, I'd auto-generate version.h from the
already existing VERSION file, or just have the Makefile add -DVERSION=
with the contents of the already existing VERSION file.  Otherwise, we
have to update both files.  And this would only tell you the upstream
base version, not the complete package version.  Why not just run rpm -q
-f /usr/bin/newrole?  And rpm -V -f /usr/bin/newrole to check that it is
unmodified from the package-provided version.

-- 
Stephen Smalley
National Security Agency


--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [PATCH] add version option to newrole

[Glauber de Oliveira Costa]

Em Quarta 08 Fevereiro 2006 10:43, Stephen Smalley escreveu:
> On Wed, 2006-02-08 at 09:34 -0200, Glauber de Oliveira Costa wrote:
> > When trying to isolate a problem today, I missed a lot an version option
> > (-v or such). This simple patch adds such an option.
> >
> > If it's of interest, such an option may be added to whole policycoreutils
> > package. (IMHO, it would be useful). With this possibility in mind, I
> > decided to  implement the version string in a separate version.h file,
> > that could be included by other packages too.
> >
> > Patch follows.
>
> If we were going to do this, I'd auto-generate version.h from the
> already existing VERSION file, or just have the Makefile add -DVERSION=
> with the contents of the already existing VERSION file.  Otherwise, we
> have to update both files.  And this would only tell you the upstream
> base version, not the complete package version.  Why not just run rpm -q
> -f /usr/bin/newrole?  And rpm -V -f /usr/bin/newrole to check that it is
> unmodified from the package-provided version.

Thank you for your comments Stephen,

Agreed, auto-generating it from VERSION file would be my choice. I'll rewrite 
it this way. About running rpm --whatever, this does not work in all 
environments. Debian based distributions does not have this options (although 
they have their own dpkg versions of it), and there are always the scenario 
in which one may be experimenting with installations directly from source, in 
which package managers are not involved at all. After all, version options 
are quite common in programs in general. 

glauber.

--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [PATCH] add version option to newrole

[Steve G]

>@@ -10,7 +10,7 @@
>  *
>  * USAGE:
>  *
>- * newrole [ -r role ] [ -t type ] [ -l level ] [ -- args ]
>+ * newrole [ -r role ] [ -t type ] [ -l level ] [ -v ] [ -- args ]
>  *
>  * BUILD OPTIONS:
>  *

Args doesn't sound descriptive to me.

>diff -upNr policycoreutils-1.29.18/newrole/version.h 
>policycoreutils-1.29.18.mine/newrole/version.h
>--- policycoreutils-1.29.18/newrole/version.h	1969-12-31 21:00:00.000000000
>-0300
>+++ policycoreutils-1.29.18.mine/newrole/version.h	2006-02-07
>16:15:47.000000000 
>-0200
>@@ -0,0 +1,4 @@
>+
>+#ifndef VERSION
>+#define VERSION "1.29.18"
>+#endif

Version is traditionally kept in config.h or the makefiles. The version isn't
unique to newrole. there are other apps in the same package that have the same
version number. If we do one, we should do them all and put the definition in a
central location like the toplevel Makefile.

-Steve

__________________________________________________
Do You Yahoo!?
Tired of spam?  Yahoo! Mail has the best spam protection around 
http://mail.yahoo.com 

--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [PATCH] add version option to newrole

[Glauber de Oliveira Costa]

Steve, thanks a lot for your comments,

Em Quarta 08 Fevereiro 2006 11:06, Steve G escreveu:
> >@@ -10,7 +10,7 @@
> >  *
> >  * USAGE:
> >  *
> >- * newrole [ -r role ] [ -t type ] [ -l level ] [ -- args ]
> >+ * newrole [ -r role ] [ -t type ] [ -l level ] [ -v ] [ -- args ]
> >  *
> >  * BUILD OPTIONS:
> >  *
>
> Args doesn't sound descriptive to me.

What do you suggest instead? -v is what is commonly associated with version 
options in most programs.

> >diff -upNr policycoreutils-1.29.18/newrole/version.h
> >policycoreutils-1.29.18.mine/newrole/version.h
> >--- policycoreutils-1.29.18/newrole/version.h	1969-12-31
> > 21:00:00.000000000 -0300
> >+++ policycoreutils-1.29.18.mine/newrole/version.h	2006-02-07
> >16:15:47.000000000
> >-0200
> >@@ -0,0 +1,4 @@
> >+
> >+#ifndef VERSION
> >+#define VERSION "1.29.18"
> >+#endif
>
> Version is traditionally kept in config.h or the makefiles. The version
> isn't unique to newrole. there are other apps in the same package that have
> the same version number. If we do one, we should do them all and put the
> definition in a central location like the toplevel Makefile.

See my mail. I stated that if the option is of interest, I may implement it in 
these other packages. This first patch was just an initial attempt. A 
version.h (though auto-generated as Stephen suggested) is still my choice. 
But I'll take a look about putting it in the Makefile. My main concern is 
being able to compile it from newrole (or whatever) directory, not from the 
top level one.
 
> -Steve

Thanks again,

glauber.

> __________________________________________________
> Do You Yahoo!?
> Tired of spam?  Yahoo! Mail has the best spam protection around
> http://mail.yahoo.com

--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.