<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">2015-08-05 13:36 GMT-07:00 Pavel Emelyanov <span dir="ltr"><<a href="mailto:xemul@parallels.com" target="_blank">xemul@parallels.com</a>></span>:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">On 08/05/2015 08:51 PM, Gabriel Guimaraes wrote:<br>
> Hi Pavel,<br>
><br>
> Thanks for the review.<br>
><br>
</span><span class="">> On Wed, Aug 5, 2015 at 4:00 AM, Pavel Emelyanov <<a href="mailto:xemul@parallels.com">xemul@parallels.com</a> <mailto:<a href="mailto:xemul@parallels.com">xemul@parallels.com</a>>> wrote:<br>
><br>
> Gabriel,<br>
><br>
> Please, find my comments inline.<br>
><br>
> > diff --git a/crtools.c b/crtools.c<br>
> > index 6af6080..521d0ff 100644<br>
> > --- a/crtools.c<br>
> > +++ b/crtools.c<br>
> > @@ -235,6 +235,7 @@ int main(int argc, char *argv[], char *envp[])<br>
> > { "enable-fs", required_argument, 0, 1065 },<br>
> > { "enable-external-sharing", no_argument, 0, 1066 },<br>
> > { "enable-external-masters", no_argument, 0, 1067 },<br>
> > + { "overlayfs", no_argument, 0, 1068 },<br>
><br>
> With AUFS workaround we ended up with auto-detection scheme -- when<br>
> meeting an AUFS mount the opts.aufs is set to true. Can we do the<br>
> same with overlayfs?<br>
><br>
><br>
> Technically we could always enable overlayfs by default, since we only do something if there<br>
> is an overlayfs mounted entry in the mountinfo table. Should I enable it always?<br>
<br>
</span>In the fill_fdlink() there's fd_parms structure with fs_type field filled from<br>
statfs() syscall. If we check for this value being overlayfs magic, we can silently<br>
imply the workaround is on and go calling fixup_overlayfs().<br>
<span class=""><br></span></blockquote><div>Per man statfs, I don't think Overlayfs has a magic number. OverlayFS just overlays two filesystems on top of each other. From statfs' point of view, a file's filesystem type is the original one, not OverlayFS.</div><div><br></div><div>More information here:</div><div><a href="https://www.kernel.org/doc/Documentation/filesystems/overlayfs.txt">https://www.kernel.org/doc/Documentation/filesystems/overlayfs.txt</a><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">
> > +static struct mount_info *__lookup_overlayfs(struct mount_info *list, char *rpath,<br>
> > + unsigned int st_dev, unsigned int st_ino,<br>
> > + unsigned int mnt_id)<br>
> > +{<br>
> > + /*<br>
> > + * Goes through all entries in the mountinfo table<br>
> > + * looking for a mount point that contains the file specified<br>
> > + * in rpath. Uses the device number st_dev and the inode number st_ino<br>
> > + * to make sure the file is correct.<br>
> > + */<br>
> > + struct mount_info *mi_ret = NULL;<br>
> > + struct mount_info *m;<br>
> > +<br>
> > + for (m = list; m != NULL; m = m->next) {<br>
> > + /* Case 1: If mnt_id is correct, don't do anything */<br>
> > + if (st_dev == m->s_dev && mnt_id == m->mnt_id)<br>
> > + return NULL;<br>
><br>
> Shouldn't this if () be under the next one, so that everything that happens<br>
> here applies to overlayfs only?<br>
><br>
><br>
> This if () is basically a check to see if the bug exists. We look for all instances of the device<br>
> st_dev and see if any of them match our mnt_id. If that's not the case, the bug exists and we want<br>
> to proceed to try to fix it.<br>
<br>
</span>OK<br>
<span class=""><br>
> I could have left this as a separate for loop to make this more clear, but this way we only use<br>
> one for loop for both. I believe it's better if we separate it into two for loops since we prevent<br>
> some unnecessary stating. I can also move this part to lookup_overlayfs, before dealing with<br>
> namespaces. What do you think?<br>
<br>
</span>Yup, makes sense to me.<br>
<br>
</blockquote></div><br></div></div>