current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 1 down vote favorite

So hi,

my goal with my code is to have a program similar to pipes in unix like $printenv | sort | less

using recursion. I'm pretty new to pipes and file descriptor manipulation so I don't know if I use them as good as I could. Also, my problem is that this code, although it works, doesn't give me the userability from less. I get the text from less but get sent back to my terminal. Why is that? I haven't bothered with error checking just yet but it'll come! So, here goes:

#include <unistd.h>
#include <stdio.h>
#include <string.h>
#include <stdlib.h>
#define AMOUNT (2)

enum read_write{READ, WRITE};
int std_out;

void rec(char **file_names, int count){
    int pid, fd[2];
    if(count >= AMOUNT){
        fd[WRITE] = std_out;
        pid = 1;
    }
    else{
        pipe(fd);
        pid = fork();
    }
    if(!pid){
        close(fd[WRITE]);
        dup2(fd[READ], STDIN_FILENO);
        rec(file_names, count+1);
    }
    else{
        close(fd[READ]);
        dup2(fd[WRITE], STDOUT_FILENO);
        execl(*(file_names+count), *(file_names+count), (char*) 0);
        if(argc==1){}
    }
}
int main(int argc, char **argv){
    char **name = malloc(1024);
    std_out = dup(STDOUT_FILENO);
    *name = "/usr/bin/emerge";
    *(name+1) = "/bin/sort";
    *(name+2) = "/usr/bin/less";
    rec(name, 0);
    return 0;
}
share|improve this question

closed as off-topic by Jamal♦ Apr 12 '14 at 6:33

This question appears to be off-topic. The users who voted to close gave this specific reason:

  • "Questions containing broken code or asking for advice about code not yet written are off-topic, as the code is not ready for review. Such questions may be suitable for Stack Overflow or Programmers. After the question has been edited to contain working code, we will consider reopening it." – Jamal
If this question can be reworded to fit the rules in the help center, please edit the question.
    
That does not make sense. Pipe is not a command. It is part of the shell syntax that allows you to redirect stdin/stdout. It is not an application. This is available in most shells even on Windows. –  Loki Astari Jul 6 '12 at 19:19
    
@loki, Of course, my code demonstrates how I'm using the system call pipe() to... pipe my in- and output between the child and parent combined with recursion. I can't see anything that doesn't make sense –  Pengtuzi Jul 6 '12 at 19:41
1  
The idea makes no sense. But sure; if you just want to mess around with pipes to see what you can do then fine. –  Loki Astari Jul 6 '12 at 19:52
    
The idea is to simulate a shell's piping as a way of learning, it made enough sense to my prof to include this as an old excercise. Since you seem confused about a shell's pipeline and the system call pipe it's understandable as how you're failing to make sense of this. –  Pengtuzi Jul 6 '12 at 20:33

1 Answer 1

up vote 4 down vote

Not sure why you need a special variable for this:

#define AMOUNT (2)

The braces just make it event harder to read and have no purpose.
This is the wrong use for macros. They are supposed to be used for conditional compilation. Prefer to use const global variable.

Global variables are a bad idea. (Note: This is different from a const global. This is changeable state. It is the fact that it is changeable that makes it bad and the program hard to analyses. const globals are fine in this regard.

int std_out;

Just as easy to pass this value as a parameter.

One variable declaration per line.

int pid, fd[2];

No coding standard out there will accept this get used to it.

Because you us a macro for amount this becomes unreadable. I have no idea when it will go either side of the branch. Eather use numeric literal 2 or a better named variable.

if(count >= AMOUNT){
    fd[WRITE] = std_out;
    pid = 1;
}
else{
    pipe(fd);
    pid = fork();
}

The fact that you are faking a pid makes the code hard to follow. Try not to re-use variables for different meanings. This is not 1972 when we were strained for space and the compiler will reuse space as appropriate if it can so you don't actually loos anything and making them different variables will make the code easier to read.

The recursive calls just makes me blood curl. Let the shell do this for you. You are going past a lets see what I can do and trying to implement technology that is already available and works correctly.

Also all failures in fork() are going to go down this loop. Which will likely cause infinite recursion and crash the program.

if(!pid){
    close(fd[WRITE]);
    dup2(fd[READ], STDIN_FILENO);
    rec(file_names, count+1);
}
share|improve this answer

Not the answer you're looking for? Browse other questions tagged or ask your own question.