Dan Price wrote:
> On Mon 28 Jan 2008 at 04:02PM, Steve Peng wrote:
>
>> The full nightly build was built successfully and testings look good as
>> well. I have posted the new incremental webrev under
>>
>> http://cr.opensolaris.org/~stevep/6351623-2
>>
>> The previous one has been moved under 6351623-1. Please take a look at
>> and if you can get back to me with any new comment
>> by cob Wed then that will great.
>>
>
> I just wanted to followup and confirm: have you done stress testing with
> 'svcadm _smf_repository_switch' a tight loop? Tested with zones?
>
Dan,
Again, thanks for your quick response.
Havn't done yet. Will do it today. What's the best way to test it in
the zone environment? Is it sufficient enough to just create zone and
reboot it multiple times?
> A couple of other minor stylistic comments; none should impact your testing.
>
>
> 1120 + * Copying the repository. If the sw_back flag is not set, we are
>
> "Copying" -> "Copy"
>
Updated.
> 1161 + configd_critical("Backend copy failed: openning %s:
> %s\n",
>
> "openning" -> "opening"
>
Updated.
> 1186 + configd_critical("Backend copy failed: copy bytes
> mismatch\n");
>
How about "incomplete copy"?
> Message is a little vague.
>
> 1237 + if (r == SQLITE_OK && (info.rs_result !=
> 1238 + REP_PROTOCOL_FAIL_NOT_FOUND && val ==
> 1239 + BACKEND_SCHEMA_VERSION))
>
> The parens here are I think unneeded, and actually are confusing.
> Code might be a lot more readable as:
>
> if (r == SQLITE_OK &&
> info.rs_result != REP_PROTOCOL_FAIL_NOT_FOUND &&
> val == BACKEND_SCHEMA_VERSION)
>
Ok, updated.
>
>
> 1246 + * Backend switch entry point and is called to perform
> 1247 + * the backend copy and switch from src to dst. First,
> 1248 + * it blocks all other clients from accessing the repository
> 1249 + * by calling backend_lock to lock the repository. Upon
> 1250 + * successful lock, coping and switching of the repository
> 1251 + * are performed.
>
> 1246 is sort of a run-on sentence...
>
Updated.
> "coping" -> "copying"
>
Updated.
> Also, please format to 76 columns or so. Ditto at line 1275.
>
Updated.
>
>
> 1299 configd_critical(
> 1300 + "Backend switch fails: integrity check
> %s: %s\n",
> 1301 + dst, errp)
> "fails" --> "failed", same at 1305.
>
Updated.
>
> 2154 + * If the system crashed during a backend switch, there
> might
> 2155 + * be a leftover transient database which containing useful
> 2156 + * information which can be used for recovery.
>
> "which containing" --> "which contains"
>
Updated.
> repcache_protocol.h:
>
> 359 + * When the flag is set to fast, the operation moves the main
> repository
> 360 + * from the default location (/etc/svc) to the tmpfs location
> 361 + * (/etc/svc/volatile). When it is set to perm, the
> operation performs
> 362 + * the reversed switch.
>
> Suggest: "When the flag is set to 'fast', move the main repository
> ^ ^ ^^^^^^^^^^^^^^^^^^^
> from the default location (/etc/svc) to the tmpfs location
> (/etc/svc/volatile).
> When it is set to 'perm', the switch is reversed.
>
Updated.
> ^ ^ ^^^^^^^^^^^^^^^^^^^
>
> Otherwise, looks good. Thanks for tackling this problem.
>
> -dp
>
>
I will send out another incremental webrev when it is ready.
-Steve
