patterncMinor
Send command and get response from Windows CMD prompt silently
Viewed 0 times
responsegetcmdwindowssilentlyandcommandfromsendprompt
Problem
Note - The response to reviews in this post is here.
The need:
Simply stated:
I needed a method to programmatically send commands to the Windows 7 (and newer)
The design:
The development environment in addition to the Windows 7 OS is an ANSI C (C99) compiler from National Instruments, and the Microsoft Windows Driver Kit for Windows 8.1.
Among the design goals was to present a very small API, including well documented and straightforward usage instructions. The result is a single exported function, with a prototype having 3 arguments. In its provided form, it is intended to be built as a DLL. The only header files I used on my environment were
For review consideration:
The code posted is complete, and I have tested it, but I am new to using
I am particularly interested in having feedback focused on the following:
Usage example:
```
#include "cmd_rsp.h"
int main(void)
{
char *buf = {0};
buf = calloc(100, 1);
if(!buf)return 0;
cmd_rsp("dir /s", &buf, 100);//where "dir /s" could be any command line
//input that results in a stdout response
printf("%
The need:
Simply stated:
_popen() for WindowsI needed a method to programmatically send commands to the Windows 7 (and newer)
Command Prompt, (aka CMD console), and return the response into a buffer without seeing a console popup. The design should provide for easy implementation into c applications needing this functionality.The design:
The development environment in addition to the Windows 7 OS is an ANSI C (C99) compiler from National Instruments, and the Microsoft Windows Driver Kit for Windows 8.1.
Among the design goals was to present a very small API, including well documented and straightforward usage instructions. The result is a single exported function, with a prototype having 3 arguments. In its provided form, it is intended to be built as a DLL. The only header files I used on my environment were
windows.h and stdlib.h.For review consideration:
The code posted is complete, and I have tested it, but I am new to using
pipes to stdin and stdout, as well as using Windows methods for CreateProcess(...). Also, because the size requirements of the response buffer cannot be known at compile time, the code also includes the ability to grow the response buffer as needed during run-time. For example, during testing I read directories recursively using dir /s from many locations. (excluding the root c:\ directory) For example:cd c:\dev && dir /s // approximately 1.8Mbyte buffer is returned on my systemI am particularly interested in having feedback focused on the following:
- Pipe creation and usage
CreateProcessusage
- Method used for dynamically growing response buffer
Usage example:
```
#include "cmd_rsp.h"
int main(void)
{
char *buf = {0};
buf = calloc(100, 1);
if(!buf)return 0;
cmd_rsp("dir /s", &buf, 100);//where "dir /s" could be any command line
//input that results in a stdout response
printf("%
Solution
The question is very interesting, and obviously required some real time to implement.
Just a few observations and suggestions:
In the declaration of cmd_rsp() the variable size might be better named chunk_size or ChunkSize to indicate it is the buffer size. The function cmd_rsp() might be altered to return the output string or NULL in case of error, this removes the need to pass in the buffer and any possible buffer
overflows.
Global Symbols
While the cmd_rsp.h file does not include the declarations of these variables and functions, the scope of these variables and functions is global and may impact names within the program itself. It is also possible that a programmer trying to maintain the code will add the variables or functions in another part of the program, this can either lead to link time problems or bugs through unintentional side affects.
If the goal of cmd_rsp.h and cmd_rsp.c is to eventually become either a static library or a dynamically linked library this is problematic practice.
Global Variables
While these variables could be declared
to write, maintain and debug. The file cmd_rsp.c is already 254 lines long, if features are added in the future by you or someone else it becomes very difficult to find where side affects may change the values of these variables.
Global Functions
In the cmd_rsp.c file there is the following code with comments:
The way the function prototypes are defined, they are global to any progam that links in cmd_rsp.c. To truely make the functions private use the
Improper Initialization of Pointers
The following declarations are allocating arrays of characters of size 1. Is this what was desired?
So the following code is throwing away a character array of size 1: (possible memory leak)
There are 2 more problems with the previous code, one is specific to the CodeReview.StackExchange.com
website:
If the desired goal was to initialize the pointers to a null value, then the proper way to do this is to include one of the following files and use the NULL macro, stddef.h, stdlib.h, or stdio.h. The second answer to this stackoverflow question provides a larger list of files that supply NULL.
If the code isn't going to include the header files for some reason see this stackoverflow question for better ways to define NULL.
The Example Usage Should Provide Proper Usage
The example doesn't test the results of the call to cmd_rsp(). A better example might be:
Reduce Complexity, Follow SRP
The Single Responsibility Principle states that every module or class should have responsibility over a single part of the functionality provided by the software, and that responsibility should be entirely encapsulated by the class. All its services should be narrowly aligned with that responsibility.
While this is primari
Just a few observations and suggestions:
In the declaration of cmd_rsp() the variable size might be better named chunk_size or ChunkSize to indicate it is the buffer size. The function cmd_rsp() might be altered to return the output string or NULL in case of error, this removes the need to pass in the buffer and any possible buffer
overflows.
Global Symbols
While the cmd_rsp.h file does not include the declarations of these variables and functions, the scope of these variables and functions is global and may impact names within the program itself. It is also possible that a programmer trying to maintain the code will add the variables or functions in another part of the program, this can either lead to link time problems or bugs through unintentional side affects.
If the goal of cmd_rsp.h and cmd_rsp.c is to eventually become either a static library or a dynamically linked library this is problematic practice.
Global Variables
While these variables could be declared
static like the functions below a better approach might be to declare them within the function cmd_rsp(const char *command, char **chunk, unsigned int size). Not only does this remove the variables from the program name space, it also makes the code easierto write, maintain and debug. The file cmd_rsp.c is already 254 lines long, if features are added in the future by you or someone else it becomes very difficult to find where side affects may change the values of these variables.
void* g_hChildStd_IN_Rd = NULL;
void* g_hChildStd_IN_Wr = NULL;
void* g_hChildStd_OUT_Rd = NULL;
void* g_hChildStd_OUT_Wr = NULL;Global Functions
In the cmd_rsp.c file there is the following code with comments:
// Private prototypes
int CreateChildProcess(const char *cmd);
int ReadFromPipe(char **rsp, unsigned int size);
char * ReSizeBuffer(char **str, unsigned int size);The way the function prototypes are defined, they are global to any progam that links in cmd_rsp.c. To truely make the functions private use the
static keyword in both the prototype and the function declaration.// Private prototypes
static int CreateChildProcess(const char *cmd);
static int ReadFromPipe(char **rsp, unsigned int size);
static char * ReSizeBuffer(char **str, unsigned int size);
static int CreateChildProcess(const char *cmd)
{
...
}Improper Initialization of Pointers
The following declarations are allocating arrays of characters of size 1. Is this what was desired?
char *buf = {0}; // main()
char *Command = {0}; // cmd_rsp()
char *tmp1 = {0}; // ReadFromPipe()
char *tmp2 = {0}; // ReadFromPipe()So the following code is throwing away a character array of size 1: (possible memory leak)
char *Command = {0};
// if(!strstr(command, rqdStr))
// {
Command = calloc(len + sizeof(rqdStr), 1);
strcat(Command, rqdStr);
strcat(Command, command);
// }There are 2 more problems with the previous code, one is specific to the CodeReview.StackExchange.com
website:
- Command is not tested to see if the calloc() actually returned memory or if there was a memory allocation error.
- Quite often on CodeReview commented out code indicates bugs and the question will be flagged as broken code. If the strstr() test is to be removed, then it shouldn't be in the question.
If the desired goal was to initialize the pointers to a null value, then the proper way to do this is to include one of the following files and use the NULL macro, stddef.h, stdlib.h, or stdio.h. The second answer to this stackoverflow question provides a larger list of files that supply NULL.
#include
char *buf = NULL;
char *Command = NULL;If the code isn't going to include the header files for some reason see this stackoverflow question for better ways to define NULL.
The Example Usage Should Provide Proper Usage
The example doesn't test the results of the call to cmd_rsp(). A better example might be:
#include // printf() and NULL
#include // EXIT_FAILURE, EXIT_SUCCESS and NULL
#include "cmd_rsp.h"
#define MY_BUFFER_SIZE 100
int main(void)
{
int status = EXIT_SUCCESS;
char *buf = NULL;
buf = calloc(MY_BUFFER_SIZE, 1);
if(!buf)return 0;
if (!cmd_rsp("dir /s", &buf, MY_BUFFER_SIZE))
{
printf("%s", buf);
}
else
{
status = EXIT_FAILURE;
}
free(buf);
return status;
}Reduce Complexity, Follow SRP
The Single Responsibility Principle states that every module or class should have responsibility over a single part of the functionality provided by the software, and that responsibility should be entirely encapsulated by the class. All its services should be narrowly aligned with that responsibility.
Robert C. Martin expresses the principle as follows:A class should have only one reason to change.While this is primari
Code Snippets
void* g_hChildStd_IN_Rd = NULL;
void* g_hChildStd_IN_Wr = NULL;
void* g_hChildStd_OUT_Rd = NULL;
void* g_hChildStd_OUT_Wr = NULL;// Private prototypes
int CreateChildProcess(const char *cmd);
int ReadFromPipe(char **rsp, unsigned int size);
char * ReSizeBuffer(char **str, unsigned int size);// Private prototypes
static int CreateChildProcess(const char *cmd);
static int ReadFromPipe(char **rsp, unsigned int size);
static char * ReSizeBuffer(char **str, unsigned int size);
static int CreateChildProcess(const char *cmd)
{
...
}char *buf = {0}; // main()
char *Command = {0}; // cmd_rsp()
char *tmp1 = {0}; // ReadFromPipe()
char *tmp2 = {0}; // ReadFromPipe()char *Command = {0};
// if(!strstr(command, rqdStr))
// {
Command = calloc(len + sizeof(rqdStr), 1);
strcat(Command, rqdStr);
strcat(Command, command);
// }Context
StackExchange Code Review Q#162546, answer score: 5
Revisions (0)
No revisions yet.