pmem / ndctl

A "device memory" enabling project encompassing tools and libraries for CXL, NVDIMMs, DAX, memory tiering and other platform memory device topics.
Other
268 stars 138 forks source link

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

Open sscargal opened 5 years ago

sscargal commented 5 years ago

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:

# ndctl --list
Unknown option: --list

 Usage: ndctl [--version] [--help] COMMAND [ARGS]
# echo $?
129
djbw commented 5 years ago

"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.

sscargal commented 5 years ago

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
djbw commented 5 years ago

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.