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

[Glauber Costa]

>> +{
>> +    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.

>> +
>> +    /*
>> +     * 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.