> On Jul 23, 2007, at 5:39 PM, Jo Rhett wrote:
>> Eric, the final lines of this patch remove something essential.
>> You're not dealing with the situation properly when the pkglist is
>> empty. That is a separate check, and relevant. However, it needs
>> to return failure to the invoker so that they know this.
>
On Jul 24, 2007, at 12:32 PM, Eric Searcy wrote:
> I would say I was handling the case where it was empty just as well
> as trunk does. However, I agree that we ought to return failure.
> We might as well do that at the beginning of the function (i.e,
> precondition: pkglist != NULL ... it's also true that it shouldn't
> be called if pkglist == NULL however).
I try to have very defensive code. Check it before you start, and
check it when you're done. What I've seen in the past is that
another check will get added in the middle of BuildCommandLine() at
some point and thus a non-empty pkglist might still not evaluate to
any packages to install, and we'd be doing the wrong thing. Given
that this is a very simple, fast test I would rather keep it in place
and be defensive by nature.
> The prior version checked to see if cmd_ptr != &resolvedcmd
> [original_len]. However, cmd_ptr was set to &resolvedcmd
> [original_len]. Then, even if pkglist == NULL, we decremented
> cmd_ptr by one! In short, we were never hitting the branch cmd_ptr
> == &resolvedcmd[original_len]; the if was always true. So, I
> removed a branch that only had one possible path, because it a)
> hadn't been being used b) didn't seem necessary.
Ouch, good catch! You are very right. Here's a patch for that.
--- package.c-0724 Tue Jul 24 12:44:55 2007
+++ package.c Tue Jul 24 12:45:56 2007
@@ -445,9 +445,6 @@
}
}
- /* Remove trailing space */
- *--cmd_ptr = '\0';
-
/* If the revised command line is the same, we have nothing to
do. */
if (cmd_ptr == &resolvedcmd[original_len])
{
@@ -456,7 +453,10 @@
}
else
{
- /* Have a full command line, so append the tail if necessary. */
+ /* Have a full command line, so remove trailing space */
+ *--cmd_ptr = '\0';
+
+ /* Append the tail if necessary. */
if (cmd_tail_len > 0)
{
strncpy(cmd_ptr, cmd_tail, &resolvedcmd[CF_BUFSIZE*2] -
cmd_ptr);
>> I'm not sure about removing ptr and reusing another pointer. I've
>> seen interesting bugs in
> Okay. I don't mind using two. I too think it makes the code hard
> to read when a pointer is reused for a different purpose; however
> reusing a string-offset pointer in a second string when you're done
> tokenizing the first string seems natural to me, but again, I don't
> mind.
Yeah, me too. I wouldn't fight hard on this one, but for now lets
get it working.
> I removed "Package manager will be invoked as %s" because it only
> has the left side of %s from ...
> functions. However, I think the line that I left should be
> cfinform (like what I was saying about shellcommands). What do you
> think?
Let's get the code working, and then give Mark a separate patch for
just the output changes. I agree with most of your logic. Frankly,
I want to make most of my Verbose be Debug instead, have the Verbose
just output the command line stuff we talked out, and have the normal
output be just the list of packages installed or not installed (or
removed).
>> On the lines of output, not including a package is notable and
>> should be reported so I changed the output from Verbose to normal
>> output.
>
> Good idea. How about cferror instead of cfinform?
*ducks* I still haven't figured out the proper ways to handle output
and need to get a clue. Have you found a good reference for me to
page into my brain?
--
Jo Rhett
senior geek
Silicon Valley Colocation
Support Phone: 408-400-0550
_______________________________________________
Bug-cfengine mailing list
[email protected]
https://cfengine.org/mailman/listinfo/bug-cfengine
