[Devel] [PATCH v5 6/6] allow for distro-specific fix ups at creation time.

[Kir Kolyshkin]

On 05/17/2013 11:54 PM, Glauber Costa wrote:
>>> +{
>>> +    char buf[STR_SIZE];
>>> +
>>> +    /* Distributions that don't need the fixup will can stop right
>>> here */
>>> +    if (!actions || !actions->ct_fixup)
>>> +        return 0;
>>> +
>>> +    if (snprintf(buf, sizeof(buf), "%s/%s", root,
>>> "/etc/rc3.d/S00vz-fixups.sh") < 0)
>> Again and again :(
>> How this snprintf can return negative value?
>>
> Will change.
>
>>> +        goto out;
>>> +
>>> +    if (open(buf, O_RDWR|O_CREAT, 0) < 0)
>>> +        goto out;
>>> +
>>> +    if (mount(actions->ct_fixup, buf, "", MS_BIND, 0) < 0)
>>> +        goto out;
>> Please remind me why are you bind-mounting rather than just copying the
>> file?
>>
> Because I believe it to be more robust this way. The user won't be able
> to accidentally delete it, mess with permissions, etc. IOW, this is to
> to make it all temporary and avoid changing the container permanently.

OK, makes sense.

>
>>> +
>>> +    /*
>>> +     * Being safe never killed anyone...
>>> +     * We bind mount instead of symlinking the original rc3 so it
>>> won't appear in the original
>>> +     * private mount.
>>> +     */
>>> +    if (snprintf(buf, sizeof(buf), "%s/%s", root,
>>> "/etc/rc5.d/S00vz-fixups.sh") < 0)
>>> +        goto out;
>>> +
>>> +    if (open(buf, O_RDWR|O_CREAT, 0) < 0)
>>> +        goto out;
>>> +
>>> +    if (mount(actions->ct_fixup, buf, "", MS_BIND, 0) < 0)
>>> +        goto out;
>> This is copy-pasting. Can you at least redo this with something like
>>
>> char *dirs[] = {"/etc/rc3.d", "/etc/rc5.d"};
>>
>> for(i = 0; i < ARRAY_SIZE(dirs); i++) {
>>          bla bla bla
>> }
>>
> Yes.
>