public inbox for linux-xfs@vger.kernel.org
 help / color / mirror / Atom feed
* [PATCH] io/mremap: modify argument errors of mremap command
@ 2016-03-16  9:31 Zorro Lang
  2016-03-16 12:31 ` Brian Foster
  0 siblings, 1 reply; 3+ messages in thread
From: Zorro Lang @ 2016-03-16  9:31 UTC (permalink / raw)
  To: xfs; +Cc: Zorro Lang

There're 2 argument errors in mremap command:
1. If run "mremap 1024 8192", it won't return error and try to
do "mremap 1024" silently.

2. The "-f" option can't be used, due to it need a new address
argument as the fifth argument of mremap() syscall(man mremap).

This patch try to fix above two problems.

Signed-off-by: Zorro Lang <zlang@redhat.com>
---

Hi,

I'm not sure pass a address argument to mremap command is right or
wrong. And maybe I should use strtoul() to instead of cvtnum() for
address argument.

At least I did some simple test with this patch on my machine, results are:
"mremap -f 139946004389888 2048"  PASS
"mremap -f 4096k 2048"            PASS
"mremap -f 1g 2048"               PASS
"mremap -f 4096 2048"             FAIL as 'Permission denied'

What do you think of this patch?

Thanks,
Zorro


 io/mmap.c         | 22 +++++++++++++++-------
 man/man8/xfs_io.8 |  5 ++++-
 2 files changed, 19 insertions(+), 8 deletions(-)

diff --git a/io/mmap.c b/io/mmap.c
index 5970069..672ee61 100644
--- a/io/mmap.c
+++ b/io/mmap.c
@@ -589,7 +589,7 @@ mremap_help(void)
 "\n"
 " Resizes the mappping, growing or shrinking from the current size.\n"
 " The default stored value is 'X', repeated to fill the range specified.\n"
-" -f -- use the MREMAP_FIXED flag\n"
+" -f <new_address> -- use the MREMAP_FIXED flag to mremap on the new address\n"
 " -m -- use the MREMAP_MAYMOVE flag\n"
 "\n"));
 }
@@ -600,15 +600,18 @@ mremap_f(
 	char		**argv)
 {
 	ssize_t		new_length;
-	void		*new_addr;
+	void		*new_addr = NULL;
 	int		flags = 0;
 	int		c;
 	size_t		blocksize, sectsize;
 
-	while ((c = getopt(argc, argv, "fm")) != EOF) {
+	init_cvtnum(&blocksize, &sectsize);
+
+	while ((c = getopt(argc, argv, "f:m")) != EOF) {
 		switch (c) {
 		case 'f':
 			flags = MREMAP_FIXED|MREMAP_MAYMOVE;
+			new_addr = (void *)cvtnum(blocksize, sectsize, optarg);
 			break;
 		case 'm':
 			flags = MREMAP_MAYMOVE;
@@ -618,7 +621,9 @@ mremap_f(
 		}
 	}
 
-	init_cvtnum(&blocksize, &sectsize);
+	if (optind != argc - 1)
+		return command_usage(&mremap_cmd);
+
 	new_length = cvtnum(blocksize, sectsize, argv[optind]);
 	if (new_length < 0) {
 		printf(_("non-numeric offset argument -- %s\n"),
@@ -626,7 +631,10 @@ mremap_f(
 		return 0;
 	}
 
-	new_addr = mremap(mapping->addr, mapping->length, new_length, flags);
+	if (!new_addr)
+		new_addr = mremap(mapping->addr, mapping->length, new_length, flags);
+	else
+		new_addr = mremap(mapping->addr, mapping->length, new_length, flags, new_addr);
 	if (new_addr == MAP_FAILED)
 		perror("mremap");
 	else {
@@ -697,9 +705,9 @@ mmap_init(void)
 	mremap_cmd.altname = "mrm";
 	mremap_cmd.cfunc = mremap_f;
 	mremap_cmd.argmin = 1;
-	mremap_cmd.argmax = 2;
+	mremap_cmd.argmax = 3;
 	mremap_cmd.flags = CMD_NOFILE_OK | CMD_FOREIGN_OK;
-	mremap_cmd.args = _("[-m|-f] newsize");
+	mremap_cmd.args = _("[-m|-f <new_address>] newsize");
 	mremap_cmd.oneline =
 		_("alters the size of the current memory mapping");
 	mremap_cmd.help = mremap_help;
diff --git a/man/man8/xfs_io.8 b/man/man8/xfs_io.8
index 33fbe6a..984b551 100644
--- a/man/man8/xfs_io.8
+++ b/man/man8/xfs_io.8
@@ -581,7 +581,7 @@ See the
 .B mmap
 command.
 .TP
-.BI "mremap [ \-f ] [ \-m ] " new_length
+.BI "mremap [ \-f <new_address> ] [ \-m ] " new_length
 Changes the current mapping size to
 .IR new_length .
 Whether the mapping may be moved is controlled by the flags passed;
@@ -589,6 +589,9 @@ MREMAP_FIXED
 .RB ( \-f ),
 or MREMAP_MAYMOVE
 .RB ( \-m ).
+.IR new_length
+specifies a page-aligned address to which the mapping must be moved. It
+can be setted to 139946004389888, 4096k or 1g etc.
 .TP
 .B mrm
 See the
-- 
2.5.0

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

^ permalink raw reply related	[flat|nested] 3+ messages in thread
* Re: [PATCH] io/mremap: modify argument errors of mremap command
  2016-03-16  9:31 [PATCH] io/mremap: modify argument errors of mremap command Zorro Lang
@ 2016-03-16 12:31 ` Brian Foster
  2016-03-16 14:00   ` Zorro Lang
  0 siblings, 1 reply; 3+ messages in thread
From: Brian Foster @ 2016-03-16 12:31 UTC (permalink / raw)
  To: Zorro Lang; +Cc: xfs

On Wed, Mar 16, 2016 at 05:31:47PM +0800, Zorro Lang wrote:
> There're 2 argument errors in mremap command:
> 1. If run "mremap 1024 8192", it won't return error and try to
> do "mremap 1024" silently.
> 
> 2. The "-f" option can't be used, due to it need a new address
> argument as the fifth argument of mremap() syscall(man mremap).
> 
> This patch try to fix above two problems.
> 
> Signed-off-by: Zorro Lang <zlang@redhat.com>
> ---

A couple nits...

> 
> Hi,
> 
> I'm not sure pass a address argument to mremap command is right or
> wrong. And maybe I should use strtoul() to instead of cvtnum() for
> address argument.
> 
> At least I did some simple test with this patch on my machine, results are:
> "mremap -f 139946004389888 2048"  PASS
> "mremap -f 4096k 2048"            PASS
> "mremap -f 1g 2048"               PASS
> "mremap -f 4096 2048"             FAIL as 'Permission denied'
> 
> What do you think of this patch?
> 
> Thanks,
> Zorro
> 
> 
>  io/mmap.c         | 22 +++++++++++++++-------
>  man/man8/xfs_io.8 |  5 ++++-
>  2 files changed, 19 insertions(+), 8 deletions(-)
> 
> diff --git a/io/mmap.c b/io/mmap.c
> index 5970069..672ee61 100644
> --- a/io/mmap.c
> +++ b/io/mmap.c
> @@ -589,7 +589,7 @@ mremap_help(void)
>  "\n"
>  " Resizes the mappping, growing or shrinking from the current size.\n"
>  " The default stored value is 'X', repeated to fill the range specified.\n"
> -" -f -- use the MREMAP_FIXED flag\n"
> +" -f <new_address> -- use the MREMAP_FIXED flag to mremap on the new address\n"

Exceeds 80 chars.

>  " -m -- use the MREMAP_MAYMOVE flag\n"
>  "\n"));
>  }
> @@ -600,15 +600,18 @@ mremap_f(
>  	char		**argv)
>  {
>  	ssize_t		new_length;
> -	void		*new_addr;
> +	void		*new_addr = NULL;
>  	int		flags = 0;
>  	int		c;
>  	size_t		blocksize, sectsize;
>  
> -	while ((c = getopt(argc, argv, "fm")) != EOF) {
> +	init_cvtnum(&blocksize, &sectsize);
> +
> +	while ((c = getopt(argc, argv, "f:m")) != EOF) {
>  		switch (c) {
>  		case 'f':
>  			flags = MREMAP_FIXED|MREMAP_MAYMOVE;
> +			new_addr = (void *)cvtnum(blocksize, sectsize, optarg);
>  			break;
>  		case 'm':
>  			flags = MREMAP_MAYMOVE;
> @@ -618,7 +621,9 @@ mremap_f(
>  		}
>  	}
>  
> -	init_cvtnum(&blocksize, &sectsize);
> +	if (optind != argc - 1)
> +		return command_usage(&mremap_cmd);
> +
>  	new_length = cvtnum(blocksize, sectsize, argv[optind]);
>  	if (new_length < 0) {
>  		printf(_("non-numeric offset argument -- %s\n"),
> @@ -626,7 +631,10 @@ mremap_f(
>  		return 0;
>  	}
>  
> -	new_addr = mremap(mapping->addr, mapping->length, new_length, flags);
> +	if (!new_addr)
> +		new_addr = mremap(mapping->addr, mapping->length, new_length, flags);
> +	else
> +		new_addr = mremap(mapping->addr, mapping->length, new_length, flags, new_addr);

Both mremap() calls above exceed 80 chars. Otherwise, this looks Ok to
me:

Reviewed-by: Brian Foster <bfoster@redhat.com>

>  	if (new_addr == MAP_FAILED)
>  		perror("mremap");
>  	else {
> @@ -697,9 +705,9 @@ mmap_init(void)
>  	mremap_cmd.altname = "mrm";
>  	mremap_cmd.cfunc = mremap_f;
>  	mremap_cmd.argmin = 1;
> -	mremap_cmd.argmax = 2;
> +	mremap_cmd.argmax = 3;
>  	mremap_cmd.flags = CMD_NOFILE_OK | CMD_FOREIGN_OK;
> -	mremap_cmd.args = _("[-m|-f] newsize");
> +	mremap_cmd.args = _("[-m|-f <new_address>] newsize");
>  	mremap_cmd.oneline =
>  		_("alters the size of the current memory mapping");
>  	mremap_cmd.help = mremap_help;
> diff --git a/man/man8/xfs_io.8 b/man/man8/xfs_io.8
> index 33fbe6a..984b551 100644
> --- a/man/man8/xfs_io.8
> +++ b/man/man8/xfs_io.8
> @@ -581,7 +581,7 @@ See the
>  .B mmap
>  command.
>  .TP
> -.BI "mremap [ \-f ] [ \-m ] " new_length
> +.BI "mremap [ \-f <new_address> ] [ \-m ] " new_length
>  Changes the current mapping size to
>  .IR new_length .
>  Whether the mapping may be moved is controlled by the flags passed;
> @@ -589,6 +589,9 @@ MREMAP_FIXED
>  .RB ( \-f ),
>  or MREMAP_MAYMOVE
>  .RB ( \-m ).
> +.IR new_length
> +specifies a page-aligned address to which the mapping must be moved. It
> +can be setted to 139946004389888, 4096k or 1g etc.
>  .TP
>  .B mrm
>  See the
> -- 
> 2.5.0
> 
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

^ permalink raw reply	[flat|nested] 3+ messages in thread
* Re: [PATCH] io/mremap: modify argument errors of mremap command
  2016-03-16 12:31 ` Brian Foster
@ 2016-03-16 14:00   ` Zorro Lang
  0 siblings, 0 replies; 3+ messages in thread
From: Zorro Lang @ 2016-03-16 14:00 UTC (permalink / raw)
  To: Brian Foster; +Cc: xfs

On Wed, Mar 16, 2016 at 08:31:26AM -0400, Brian Foster wrote:
> On Wed, Mar 16, 2016 at 05:31:47PM +0800, Zorro Lang wrote:
> > There're 2 argument errors in mremap command:
> > 1. If run "mremap 1024 8192", it won't return error and try to
> > do "mremap 1024" silently.
> > 
> > 2. The "-f" option can't be used, due to it need a new address
> > argument as the fifth argument of mremap() syscall(man mremap).
> > 
> > This patch try to fix above two problems.
> > 
> > Signed-off-by: Zorro Lang <zlang@redhat.com>
> > ---
> 
> A couple nits...
> 
> > 
> > Hi,
> > 
> > I'm not sure pass a address argument to mremap command is right or
> > wrong. And maybe I should use strtoul() to instead of cvtnum() for
> > address argument.
> > 
> > At least I did some simple test with this patch on my machine, results are:
> > "mremap -f 139946004389888 2048"  PASS
> > "mremap -f 4096k 2048"            PASS
> > "mremap -f 1g 2048"               PASS
> > "mremap -f 4096 2048"             FAIL as 'Permission denied'
> > 
> > What do you think of this patch?
> > 
> > Thanks,
> > Zorro
> > 
> > 
> >  io/mmap.c         | 22 +++++++++++++++-------
> >  man/man8/xfs_io.8 |  5 ++++-
> >  2 files changed, 19 insertions(+), 8 deletions(-)
> > 
> > diff --git a/io/mmap.c b/io/mmap.c
> > index 5970069..672ee61 100644
> > --- a/io/mmap.c
> > +++ b/io/mmap.c
> > @@ -589,7 +589,7 @@ mremap_help(void)
> >  "\n"
> >  " Resizes the mappping, growing or shrinking from the current size.\n"
> >  " The default stored value is 'X', repeated to fill the range specified.\n"
> > -" -f -- use the MREMAP_FIXED flag\n"
> > +" -f <new_address> -- use the MREMAP_FIXED flag to mremap on the new address\n"
> 
> Exceeds 80 chars.
> 
> >  " -m -- use the MREMAP_MAYMOVE flag\n"
> >  "\n"));
> >  }
> > @@ -600,15 +600,18 @@ mremap_f(
> >  	char		**argv)
> >  {
> >  	ssize_t		new_length;
> > -	void		*new_addr;
> > +	void		*new_addr = NULL;
> >  	int		flags = 0;
> >  	int		c;
> >  	size_t		blocksize, sectsize;
> >  
> > -	while ((c = getopt(argc, argv, "fm")) != EOF) {
> > +	init_cvtnum(&blocksize, &sectsize);
> > +
> > +	while ((c = getopt(argc, argv, "f:m")) != EOF) {
> >  		switch (c) {
> >  		case 'f':
> >  			flags = MREMAP_FIXED|MREMAP_MAYMOVE;
> > +			new_addr = (void *)cvtnum(blocksize, sectsize, optarg);
> >  			break;
> >  		case 'm':
> >  			flags = MREMAP_MAYMOVE;
> > @@ -618,7 +621,9 @@ mremap_f(
> >  		}
> >  	}
> >  
> > -	init_cvtnum(&blocksize, &sectsize);
> > +	if (optind != argc - 1)
> > +		return command_usage(&mremap_cmd);
> > +
> >  	new_length = cvtnum(blocksize, sectsize, argv[optind]);
> >  	if (new_length < 0) {
> >  		printf(_("non-numeric offset argument -- %s\n"),
> > @@ -626,7 +631,10 @@ mremap_f(
> >  		return 0;
> >  	}
> >  
> > -	new_addr = mremap(mapping->addr, mapping->length, new_length, flags);
> > +	if (!new_addr)
> > +		new_addr = mremap(mapping->addr, mapping->length, new_length, flags);
> > +	else
> > +		new_addr = mremap(mapping->addr, mapping->length, new_length, flags, new_addr);
> 
> Both mremap() calls above exceed 80 chars. Otherwise, this looks Ok to
> me:
> 
> Reviewed-by: Brian Foster <bfoster@redhat.com>

Hi Brian,

Thanks for your review. I will send a V2 patch to fix these "exceed 80
chars" errors.

Thanks,
Zorro

> 
> >  	if (new_addr == MAP_FAILED)
> >  		perror("mremap");
> >  	else {
> > @@ -697,9 +705,9 @@ mmap_init(void)
> >  	mremap_cmd.altname = "mrm";
> >  	mremap_cmd.cfunc = mremap_f;
> >  	mremap_cmd.argmin = 1;
> > -	mremap_cmd.argmax = 2;
> > +	mremap_cmd.argmax = 3;
> >  	mremap_cmd.flags = CMD_NOFILE_OK | CMD_FOREIGN_OK;
> > -	mremap_cmd.args = _("[-m|-f] newsize");
> > +	mremap_cmd.args = _("[-m|-f <new_address>] newsize");
> >  	mremap_cmd.oneline =
> >  		_("alters the size of the current memory mapping");
> >  	mremap_cmd.help = mremap_help;
> > diff --git a/man/man8/xfs_io.8 b/man/man8/xfs_io.8
> > index 33fbe6a..984b551 100644
> > --- a/man/man8/xfs_io.8
> > +++ b/man/man8/xfs_io.8
> > @@ -581,7 +581,7 @@ See the
> >  .B mmap
> >  command.
> >  .TP
> > -.BI "mremap [ \-f ] [ \-m ] " new_length
> > +.BI "mremap [ \-f <new_address> ] [ \-m ] " new_length
> >  Changes the current mapping size to
> >  .IR new_length .
> >  Whether the mapping may be moved is controlled by the flags passed;
> > @@ -589,6 +589,9 @@ MREMAP_FIXED
> >  .RB ( \-f ),
> >  or MREMAP_MAYMOVE
> >  .RB ( \-m ).
> > +.IR new_length
> > +specifies a page-aligned address to which the mapping must be moved. It
> > +can be setted to 139946004389888, 4096k or 1g etc.
> >  .TP
> >  .B mrm
> >  See the
> 
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

^ permalink raw reply	[flat|nested] 3+ messages in thread
end of thread, other threads:[~2016-03-16 14:00 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-03-16  9:31 [PATCH] io/mremap: modify argument errors of mremap command Zorro Lang
2016-03-16 12:31 ` Brian Foster
2016-03-16 14:00   ` Zorro Lang

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox