Hi Channing,
Thanks for your code review. My comments are embedded.
Channing Lovely wrote:
> Hi Sanjay,
>
> I have a couple comments.
>
> 1. The original code runs devfsadm
>
> 218 /usr/sbin/devfsadm -i lofi 2> /dev/msglog
> 219 if [ -b /devices/pseudo/lofi at 0:1 ]
> 220 then
> 221 cnt=2
> 222 else
> 223 cnt=`expr $cnt + 1`
> 224 fi
>
> and I don't see your code creating the lofi devices. I don't know if
> these devices are used later, but this is a difference in
> functionality here
The purpose of the code change is to get rid of devfsadm creating the
devices. Notice that the command, lofiadm explicitly creates the devices.
>
> 2. I am not sure the new code handles errors correctly
>
> the original code ran the lofiadm's and mounts in an if loop, and then
> if they failed they broke out without setting the MOUNTED variable
>
> 235 /sbin/mount -F hsfs -o ro /dev/lofi/1 /usr
> 236 if [ $? -ne 0 ]
> 237 then
> 238 echo "/usr MOUNT FAILED!"
> 239 break
> 240 fi
>
> which was checked
>
> 254 if [ "$MOUNTED" != "1" ]
> 255 then
> 256 echo "** FATAL **: Unable to mount Live image!"
> 257 exit $SMF_EXIT_ERR_FATAL
> 258 fi
>
> and the script exits if the mounts failed.
>
> Your suggested change runs the lofiadm and mounts inline,
>
> 214 /usr/sbin/lofiadm -a /.cdrom/solaris.zlib /dev/lofi/1 ||
> break
> 215 /sbin/mount -F hsfs -o ro /dev/lofi/1 /usr 2>/dev/msglog
> 216 if [ $? -ne 0 ]
> 217 then
> 218 echo "/usr MOUNT FAILED!"
> 219 break
> 220 fi
>
> but the break here doesn't exit the script, and the MOUNTED variable
> is not checked (or used), so even if the failure occurs, the script
> continues to run. I am not sure that is what you want.
>
Good catch. I will fix it.
-Sanjay
> I think that you want an exit instead of break at line 219 and 228
>
> Channing
>
>
> Sanjay Nadkarni wrote:
>> This addresses bug 532 as well as 838.
>> http://cr.opensolaris.org/~nadkarni/live-fs-root
>>
>> -Sanjay
>>
>> _______________________________________________
>> caiman-discuss mailing list
>> caiman-discuss at opensolaris.org
>> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
>>
>
