Hi John.
Thanks for your quick reply.
On 07/ 1/10 12:35 PM, John Fischer wrote:
Jack,
I had a little trouble accessing the code review. It finally worked
when I started from
the ~schwartz directory and went from there. Very strange. Anyhow...
It seems that the be_has_grub() code starting at line 538 and
continuing through line
580 could be condensed into a single be_has_grub() if block in
lib/libbe/be_activate.c.
The be_get_pkg_version call on line 549 uses the pkg_name determined by
the first if(be_has_grub()). So if I was to consolidate, I would have
to use the ?: operator as:
if ((ret = be_get_pkg_version(
(be_has_grub() ? BE_GRUB_PKG_NAME : BE_BOOTBLK_PKG_NAME),
tmp_mntpt, pkg_version, sizeof (pkg_version))) != BE_SUCCESS) {
I thought about this and felt that ditching the ?: and having an extra
if above made things clearer. I can change to the ?: operator if you
feel strongly about this; just let me know.
In be_utils.c you introduce yet another "system()" function. There
are several of
these within the source tree. I am a little concerned of yet another
one. Although
I do not see anything wrong in the new one. Perhaps some
consolidation of all
these is in order.
Well, you are right about there being many different system() calls. (I
quickly counted 8 excluding mine and excluding system() proper.) Most
of the pre-existing ones duplicate each other; they redirect stdout to
the bit bucket and retrieve stderr to log it.
Mine can do that too, and it does in some places. But I also need it to
retrieve separate stdout and stderr streams in some cases, which is why
I wrote it.
So IMNSHO I think the others can be consolidated with each other (or be
redone to use mine as a backend), but mine needs to stay as it offers
needed functionality the others don't.
I would file a bug to consolidate all of these calls, if it wasn't for
all of the redesign we're already doing. Maybe consolidation can be
done as part of the work currently going on. If we end up with this
many system() calls after we're done, we're in trouble.
Thanks again, John.
Jack
Thanks,
John
On 07/ 1/10 11:26 AM, Jack Schwartz wrote:
Hi everyone.
Here is a webrev for the following beadm or libbe library bugfixes:
6495 <http://defect.opensolaris.org/bz/show_bug.cgi?id=6495> libbe
should capture and give useful error when installgrub, or ict.py fails.
15375 <http://defect.opensolaris.org/bz/show_bug.cgi?id=15375> libbe
changes to utilize new versioning features of installboot and installgrub
15404 <http://defect.opensolaris.org/bz/show_bug.cgi?id=15404> beadm
activate should run installboot on SPARC
16409 <http://defect.opensolaris.org/bz/show_bug.cgi?id=16409> libbe
error messages need internationalization
16429 <http://defect.opensolaris.org/bz/show_bug.cgi?id=16429> Remove
be_get_last_zone_be_callback
http://cr.opensolaris.org/~schwartz/100701.1/webrev/
Most of these changes are needed as part of the Snap -> ON project.
Note that 15375 and 15404 depend on installboot and installgrub
changes being made by another team (6944352). I have been using test
versions of installboot and installgrub to verify my changes.
Tested on SPARC and X86 by creating and then activating new BEs, then
running installboot or installgrub on the same BEs and looking for
messages that show the right thing was done by beadm. This done with
and without BE_PRINT_ERR set to true.
Please send your feedback by Tuesday 7/6 COB.
Thanks,
Jack
P.S. Note: even though these changes are being reviewed against
slim_source for clarity, they will be going back to Glenn's ON-based
gate for putback to ON.
P.P.S. Thanks in advance to Evan for agreeing to review these changes.
_______________________________________________
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
