I have sent you a pointer to what might an acceptable way. We can continue this discussion offline.

-Sanjay


On 07/14/10 08:11 PM, Jack Schwartz wrote:
Hi Sanjay.

On 07/14/10 05:34 PM, sanjay nadkarni wrote:

    On 07/14/10 04:39 PM, Evan Layton wrote:

        Hi Jack,

        I do still have some concerns with the complexity of be_run_cmd()
        and how maintainable it will be. However as we discussed I don't
        have a better suggestion on dealing with the need to capture
        stdout
        and stderr separately.

        One nit related to my maintainability comment above is that it
        might be helpful to put a comment block (starting at line 3011)
        that gives a description of how the pollfds stuff works for
        grabbing both stdout and strerr.

        As per our phone conversation I'm fine with this updated set of
        changes.

    I agree with Evan.  The code is way too complicated.  It is clear
    that popen does not do the job that you want, why not simply use
    fork() and exec and capture the stdout, stderr and stdin from that
    process.

Way too complicated? I've explained why I need to do what I do in other emails... Almost half of the proper code is error handling.

Use of fork() and exec() (and dup()) would be even more complicated. In my solution one popen() call and one mkfifo() call do the work of the fork(), exec(), 2 fdopen()s, 2 dup2()s and four close()s. I have the test program to prove it. And I would still have to call poll() to know when to read the pipes, else the deadlock I talked about earlier.

    So..the caller passes in an array of 3 ints which can be used to
    pass back.

Are the three ints file descriptors for stdin, stdout, stderr? Well, we could fork and exec, and have stdout and stderr go to files -- we don't need stdin right now -- then we would open the files to read them later. However, we agreed at the staff meeting a few weeks ago that writing to a file was hacky and not desired. And that was discussed in the context of using popen where we would only have to write one file for stderr.

If the file descriptors went to fifos instead of files, we would have to deal with a two mkfifo() calls (my current solution has only one) and we would still have to use poll() to prevent the deadlocks.

So... could it be simpler? Maybe, but mine is definitely not as complicated as it could be. IMHO I still think it is the best solution.

  Thanks,
  Jack

    =Sanjay

        Thanks,
        -evan

        On 7/10/10 5:45 PM, Jack Schwartz wrote:

            Hi everyone.

            This webrev supercedes the one I published a week ago,
            because the
            situation with SNAP->ON integration has changed. (Thanks
            to those who
            reviewed the last one!) It also includes some new changes.

            Webrev:
            http://cr.opensolaris.org/~schwartz/100710.1/webrev/

            Webrev vs old one (for those who reviewed the last one):
            http://cr.opensolaris.org/~schwartz/100710.1/webrev.diff/

            Please review by Weds 7/14 COB.

            Changes:
            - Since we cannot be sure 6944352, which addresses changes
            we needed for
            installboot and installgrub, would be done in time to make
            the ON cutoff
            for feature freeze, bugs related to that fix have been
            removed. Added is
            support for labeled branded zones, and fixes to
            be_create_menu to return
            proper errors.
            - Error message for BE_ERR_UNKNOWN is not "Unknown error"
            not "Unknown
            external error" as all external commands are now covered
            by other errors.
            - Cleanup of be_create_menu() error handling for 16512 is
            new.
            - Addition in libbe.h of "labeled" in
            BE_ZONE_SUPPORTED_BRANDS for 13865
            is new.

            Note: The new be_run_cmd function in be_utils.c (the bulk
            of original
            be_utils.c changes) has not changed.

            Tested:
            Labeled zones:
            - installed trusted solaris and created a zone. Then
            verified that the
            zone (and files it had) was properly handled in a new BE.
            - Ran regression test of libbe test suite from the STC
            gate and verified
            that there were no new errors.

            Messaging of external commands:
            Hid ict.py, deleted menu.lst and tried to create a new
            menu to check
            error output if splash screen could not be added to new
            menu.lst.
            Hid installgrub, changed capability file to force
            installgrub call, and
            then did beadm activate to check error output when
            installgrub could not
            be executed.

            Modules I changed are lint clean. All files are hg nits
            clean.

            Thanks for your time,
            Jack

            _______________________________________________
            caiman-discuss mailing list
            [email protected]
            http://mail.opensolaris.org/mailman/listinfo/caiman-discuss


        _______________________________________________
        caiman-discuss mailing list
        [email protected]
        http://mail.opensolaris.org/mailman/listinfo/caiman-discuss


    _______________________________________________
    caiman-discuss mailing list
    [email protected]
    http://mail.opensolaris.org/mailman/listinfo/caiman-discuss



_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss

Reply via email to