[CRIU] [RFC PATCH 0/5] introduce --enable-fs cli option

[Oleg Nesterov]

On 04/09, Pavel Emelyanov wrote:
>
> On 04/09/2015 08:46 PM, Oleg Nesterov wrote:
> > Hello, and sorry for delay.
> >
> > Almost the same as V1, just some cosmetics changes. Seems to work.
> >
> > I decided to keep mnt_entry->fsname and avoid the "clever" games with
> > dump/resore auto fsnames once. Because:
> >
> > 	1. I am lazy and you seem to agree with ->fsname ;)
> >
> > 	2. Your protobuf/mnt.proto is stupid (wrong afaics) and this
> > 	   means that I need to write more changes...
>
> Can you elaborate on this? :)

Which part? lazy or stupid ? ;)

> > 	   Why do we expose "enum fstype" to protobufs at all? Please
> > 	   note that typeof(mnt_entry->fstype) is uint32, not "fstype".
>
> Yes, we have this fact mentioned on the "what's bad with v1 images" page.
> We want it to be the "fstype".

Yes, and thus it is not clear what could I do if I wanted to implement
"dump/restore auto fs names once".

Because a single FSTYPE__AUTO code can't work in this case. This is trivial,
just we need


	#define FSTYPE__AUTO		1024	// random big value
	static int fstype_auto_cur = FSTYPE__AUTO;

	fscode_is_auto(code)
	{
		return code >= FSTYPE__AUTO;
	}

and find_fstype_by_name() should use fstype_auto_cur++ if fsname_is_auto().

But then it is not clear how we can dump/restore this dynamic ->fstype.

Everything should work just fine exactly because mnt_entry->fstype is not
"fstype". But since this typeof() looks like mistake, probably we should
not abuse mnt_entry->fstype this way.


> > Pavel, I didn't convert fstypes[] into the list as you suggested. Can
> > I do this in a separate patch?
>
> Sure!

Thanks! see below...

> > Because personally I think this makes
> > no sense and this (trivial) code will look worse, so we can discuss
> > this simple change separately, including the cosmetic issues like the
> > necessary ctor for non-auto entries.
>
> But current implementation doesn't require this array to be bigger than
> it actually is -- you don't add custom fsnames in it.

Not sure I understand... or perhaps you misunderstood me. I meant that
even if we turn fstypes[] into list_head, we still need fstypes_bultin[]
and constructor which walks this array and adds everything into the list.

Plus other cosmetic issues. Just for example, the "special" fstypes[0]
already looks bad too me. But a special list_first_entry(fstypes) looks
even worse imo.

So imho this change should start with "[PATCH 1/2] extract fstypes[0]
into fstype_unsupported".

But note that all these issues are absolutely trivial, cosmetic, and
subjective. So I'd like to discuss this separately, and redo this change
if you do not like (say) fstype_unsupported or something else.

> > TODO: resolve_source() && FSTYPE__AUTO. I'll resend my hack later, if/
> > when you accept this series. This needs another (more) discussion too.
>
> TODO2: Add a test (even trivial) for this functionality. Please. All this
> stuff is quite easy to break, so any help in catching simple mistakes are
> really helpful.

OK... agreed. But probably I will need your help... Right now it is not
clear to me how can I write this test-case. It needs to mount the unsupported
filesystem somehow, but how can it know that (say) hugetlbfs a) is still
not supported and b) CONFIG_HUGETLBFS is enabled? I'll try to think.

Oleg.