hi David thanks for the review, comments inline below...
David Bustos wrote: > Quoth Alan Maguire on Mon, Jun 23, 2008 at 05:05:13PM +0100: > >> http://cr.opensolaris.org/~amaguire/smf_ordered_values/ >> > > cmd/svc/configd/backend.c > 379: Please add that the variable is implicitly protected by > bes[BACKEND_TYPE_NORMAL]->be_lock . > > done. > 1024-27: This duplicates the table definition at 267. Please note > this in the comment at 264. > > sure. > 1029: This duplicates the index definition at 301. Please note this > in a comment near 295. > > done. > 1697: I don't understand why we would need to check whether the > database has been upgraded without upgrading it. Please explain it > to me. > > The logic here is that during boot, we want to determine as soon as possible if the repository is already upgraded, as if it is, we can use the value-ordered SELECT statement when retrieving values. By doing the upgrade check here, we catch the fact that the repository has been upgraded, set be_normal_upgraded to TRUE, and thus property retrieval will determine that we can use the value_order column in the SELECT statement used in fill_property_callback() (in file_object.c). I've elaborated a bit in the comment here and above backend_check_upgrade(), hopefully it's a bit clearer now. > 2374: The code at 2360 detects non-global zone boot with > writeable_persist and an uninitialized nonpersistent repository. If > you are also trying to act only when a non-global zone is booted, > why don't you use the same test? And why don't you need the backend > to be locked like the code at 2363 does? > > great catch - I've moved the backend_check_upgrade() into the lock-protected code block. > cmd/svc/configd/configd.c > 391: I suspect we should always prepend "svc.configd: ", in case the > next programmer who calls configd_info() isn't as careful about > making sure the message is clearly related to the SMF repository as > you. We could probably do this later under a separate bug, though. > > good idea - I've added a prefix "svc.configd: " to the configd_info() messages. > cmd/svc/configd/object.c > 311-4: I suspect this would be clearer to people who don't know > backend.c very well as "since it's upgraded as soon as the backend > is writable." You don't have to change it, though. > > > fixed - I think that is definitely clearer. Webrev is updated and I've retested the resultant code changes (zones upgrade and repository upgrade messaging). Thanks again for the review! http://cr.opensolaris.org/~amaguire/smf_ordered_values/ Alan
