swapon: fix -d short option

swapon: fix -d short option

[Robert Milasan]

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

When -d option is used with swapon, is expected that there is an equal
'=' which at least according to the man page it doesn't make sense or
it's not properly explained.

If we use -d as we should, then swapon takes 'once' or 'pages' as an
actual device:

dhcp33:~ # swapon -p -2 -d once /dev/sdb1
swapon: stat failed once: No such file or directory

The short option -d, should be something like "swapon -d once ...." not
"swapon -d=once ...."

I've attached the patch to fix this small issue.


-- 
Robert Milasan

L3 Support Engineer
SUSE Linux (http://www.suse.com)
email: rmilasan@suse.com
GPG fingerprint: B6FE F4A8 0FA3 3040 3402  6FE7 2F64 167C 1909 6D1A

[-- Attachment #2: swapon_fix-d-short-option.patch --]
[-- Type: text/x-patch, Size: 631 bytes --]

diff --git a/sys-utils/swapon.c b/sys-utils/swapon.c
index 8609ea2..20baea3 100644
--- a/sys-utils/swapon.c
+++ b/sys-utils/swapon.c
@@ -846,12 +846,9 @@ int main(int argc, char *argv[])
 		case 'd':
 			discard |= SWAP_FLAG_DISCARD;
 			if (optarg) {
-				if (*optarg == '=')
-					optarg++;
-
-				if (strcmp(optarg, "once") == 0)
+				if (strncmp(optarg, "once", 4) == 0)
 					discard |= SWAP_FLAG_DISCARD_ONCE;
-				else if (strcmp(optarg, "pages") == 0)
+				else if (strncmp(optarg, "pages", 5) == 0)
 					discard |= SWAP_FLAG_DISCARD_PAGES;
 				else
 					errx(EXIT_FAILURE, _("unsupported discard policy: %s"), optarg);

Re: swapon: fix -d short option

[Karel Zak]

On Tue, Oct 21, 2014 at 10:02:46AM +0200, Robert Milasan wrote:
> When -d option is used with swapon, is expected that there is an equal
> '=' which at least according to the man page it doesn't make sense or
> it's not properly explained.

Well, in the man page there was incorrectly extra space between -d and
the optional argument. Fixed yesterday.

> If we use -d as we should, then swapon takes 'once' or 'pages' as an
> actual device:
> 
> dhcp33:~ # swapon -p -2 -d once /dev/sdb1
> swapon: stat failed once: No such file or directory
> 
> The short option -d, should be something like "swapon -d once ...." not
> "swapon -d=once ...."

What? The argument for -d is optional, it's reason why we care about
'='. I think it's obvious from the code.

   swapon -p -2 -d /dev/sdb1 

is pretty valid.

Note that -d,--discard has been implemented in 2010, and extended by
'once' and 'pages' later in 2013.

> I've attached the patch to fix this small issue.

Sorry, but the patch does not make sense.

    Karel

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com

Re: swapon: fix -d short option

[Robert Milasan]

On Tue, 21 Oct 2014 11:55:25 +0200
"Karel Zak" <kzak@redhat.com> wrote:

> On Tue, Oct 21, 2014 at 10:02:46AM +0200, Robert Milasan wrote:
> > When -d option is used with swapon, is expected that there is an
> > equal '=' which at least according to the man page it doesn't make
> > sense or it's not properly explained.
> 
> Well, in the man page there was incorrectly extra space between -d and
> the optional argument. Fixed yesterday.
> 
> > If we use -d as we should, then swapon takes 'once' or 'pages' as an
> > actual device:
> > 
> > dhcp33:~ # swapon -p -2 -d once /dev/sdb1
> > swapon: stat failed once: No such file or directory
> > 
> > The short option -d, should be something like "swapon -d once ...."
> > not "swapon -d=once ...."
> 
> What? The argument for -d is optional, it's reason why we care about
> '='. I think it's obvious from the code.
> 
>    swapon -p -2 -d /dev/sdb1 
> 
> is pretty valid.
> 
> Note that -d,--discard has been implemented in 2010, and extended by
> 'once' and 'pages' later in 2013.
> 
> > I've attached the patch to fix this small issue.
> 
> Sorry, but the patch does not make sense.
> 
>     Karel
> 

Sorry, but the option doesn't make sense eider. First is not documented
properly or at all and with my patch -d option without any argument
still works.

Thanks anyway!

-- 
Robert Milasan

L3 Support Engineer
SUSE Linux (http://www.suse.com)
email: rmilasan@suse.com
GPG fingerprint: B6FE F4A8 0FA3 3040 3402  6FE7 2F64 167C 1909 6D1A

Re: swapon: fix -d short option

[Karel Zak]

On Tue, Oct 21, 2014 at 12:07:41PM +0200, Robert Milasan wrote:
> On Tue, 21 Oct 2014 11:55:25 +0200
> "Karel Zak" <kzak@redhat.com> wrote:
> 
> > On Tue, Oct 21, 2014 at 10:02:46AM +0200, Robert Milasan wrote:
> > > When -d option is used with swapon, is expected that there is an
> > > equal '=' which at least according to the man page it doesn't make
> > > sense or it's not properly explained.
> > 
> > Well, in the man page there was incorrectly extra space between -d and
> > the optional argument. Fixed yesterday.
> > 
> > > If we use -d as we should, then swapon takes 'once' or 'pages' as an
> > > actual device:
> > > 
> > > dhcp33:~ # swapon -p -2 -d once /dev/sdb1
> > > swapon: stat failed once: No such file or directory
> > > 
> > > The short option -d, should be something like "swapon -d once ...."
> > > not "swapon -d=once ...."
> > 
> > What? The argument for -d is optional, it's reason why we care about
> > '='. I think it's obvious from the code.
> > 
> >    swapon -p -2 -d /dev/sdb1 
> > 
> > is pretty valid.
> > 
> > Note that -d,--discard has been implemented in 2010, and extended by
> > 'once' and 'pages' later in 2013.
> > 
> > > I've attached the patch to fix this small issue.
> > 
> > Sorry, but the patch does not make sense.
> > 
> >     Karel
> > 
> 
> Sorry, but the option doesn't make sense eider. First is not documented

current git tree:

-d, --discard[=policy]
      Enable  swap  discards, if the swap backing device supports the discard or trim operation.
      This may improve performance on some Solid State Devices, but often it does not.  The option
      allows  one to select between two available swap discard policies: --discard=once to perform
      a single-time discard operation for the whole swap area at  swapon;  or  --discard=pages  to
      discard  freed swap pages before they are reused, while swapping.  If no policy is selected,
      the default behavior is to enable both discard types.  The /etc/fstab mount options discard,
      discard=once, or discard=pages may also be used to enable discard flags.

Maybe we can add something like

     Note that the optional policy argument cannot be separated from the -d option by a space, 
     the correct form is for example '-d=pages'.

to make it more obvious.


> properly or at all and with my patch -d option without any argument
> still works.

You want to use 

   "-d once"

for optional arguments, but this is not compatible with libc getopt().
We use '=' on many other paces... because:

man getopt:
  Two colons mean an option takes an optional arg; if there is text in the 
  current argv-element (i.e., in the same word as the option name itself, 
                                     ^^^^^^^^^
  for example, "-oarg"), then it is returned in optarg, otherwise optarg
  is set to zero.  


for example ("once" is swapfile name):

 mkswap once
 swapon -d once

I think -d=once to specify discard policy is more robust.

    Karel

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com

Re: swapon: fix -d short option

[Benno Schulenberg]

On Tue, Oct 21, 2014, at 12:38, Karel Zak wrote:
> Maybe we can add something like
> 
>      Note that the optional policy argument cannot be separated from the -d option by a space, 

It shouldn't be needed to mention this.  It is just common sense that
when an option argument is optional, you cannot separate it from the
option itself by any whitespace because then the parsing cannot decide
whether the thing is the optional option argument or simply another
plain argument.

>      the correct form is for example '-d=pages'.

Another correct form is '-dpages'.  For the short forms of the options
the '=' is not required.

Benno

-- 
http://www.fastmail.fm - Faster than the air-speed velocity of an
                          unladen european swallow