ndctl icon indicating copy to clipboard operation
ndctl copied to clipboard

strcmp() should be strcasecmp() for command-line argument and option processing

Open sscargal opened this issue 6 years ago • 3 comments

In ndctl v66 and earlier, command-line argument and option processing is very strict as we use strcmp(). See /ndctl/namespace.c for example.

static int do_xaction_namespace(const char *namespace,
		enum device_action action, struct ndctl_ctx *ctx,
		int *processed)
{
[...snip...]
				if (strcmp(namespace, "all") != 0
						&& strcmp(namespace, ndns_name) != 0)
					continue;

This causes undesirable behavior for the user. For example, the following works as expected because it adheres to the documentation and code implementation:

# ndctl list --regions --bus=all
[
  {
    "dev":"region1",
    "size":1623497637888,
    "available_size":0,
    "max_available_extent":0,
    "type":"pmem",
    "iset_id":-2506113243053544244,
    "persistence_domain":"memory_controller"
  },
  {
    "dev":"region0",
    "size":1623497637888,
    "available_size":1623497637888,
    "max_available_extent":1623497637888,
    "type":"pmem",
    "iset_id":3259620181632232652,
    "persistence_domain":"memory_controller"
  }
]

But if we run the following command, we get no output, no error, and no indication the user did anything wrong (except camel-case the All option):

# ndctl list --regions --bus=All
#                   // No output/response from ndctl
# echo $?      // Exits gracefully!!!
0
#

The user doesn't know what to do here. This is a major issue for scripts and automated deployment environments such as Puppet, Ansible, etc as the ndctl command exited gracefully even though it did nothing. Most scripts check the return code from commands to know if it was successful or failed with an error.

For this issue, we could either:

  • Accept case insensitive arguments and options using strcasecmp() instead of strcmp() (where it makes sense to do so). This would be the easiest to implement while maintaining current functionality, or
  • Return the help/usage information and an error (129) rather than (0), like we do with invalid arguments/options. If the user provides incorrect arguments or options, ndctl does return the usage and an error (129), eg:
# ndctl --list
Unknown option: --list

 Usage: ndctl [--version] [--help] COMMAND [ARGS]
# echo $?
129

sscargal avatar Aug 28 '19 15:08 sscargal

"all" is a specific keyword, but the bus is otherwise allowed to be named by a driver specific "provider" name which is free form. So, "ndctl list --bus=All" is asking ndctl to find a bus with the provider name of "All".

A third option is to just return ENOENT on empty "ndctl list" results to say that it failed to list anything rather than it successfully ran with the given filter.

djbw avatar Aug 28 '19 17:08 djbw

Is that also true for Regions and Namespaces?

# ndctl destroy-namespace -f All
error destroying namespaces: No such device or address
destroyed 0 namespaces

# ndctl destroy-namespace -f all
destroyed 4 namespaces

sscargal avatar Aug 28 '19 19:08 sscargal

destroy-namespace is operating on existing namespaces. create-namespace is a single operation where ndctl tries to search for a capacity to satisfy that single request if the user does not specify it.

Consider these invocations:

ndctl destroy-namespace -r all all ndctl create-namespace -r all

In the destroy-case there is a list of targets after the "-r all" filter parameter. create-namespace does not support a list of targets because the targets don't exist yet. I do agree the man page does not make this distinction so that can be cleaned up, but this otherwise needs an explicit option to tell ndctl to repeat the create operation.

djbw avatar Aug 28 '19 22:08 djbw