Yeah I agree ;) But I would really like for this class to be used more generically so having a list of filesystems here would probably make it hard for that. We could also have two methods, one similar to this one given the "raw" filesystem type, and one that would give an enum entry from a so called "known" list, but would return "unknown" for a filesystem type not in the list, then you would have access to both? What do you think?

No, I only put them here for completeness sake. Basically I sat down with the fstab man page and the header file documenting the mntent structure and based this class around that, but if this code should be exported an made generally available then we should probably add these, what do you think? Should I remove them now, and then we can always add them if needed?

remove

store this in the entry, otherwise you pay the cost on every access

wrap long lines, also below.

Whats wrong with mountPoint? Its used everywhere else here, and is a well known term.

should be const now

add temporary for fstype

detaches m_fstabCache, also below.

missing _k_updateMtabMountPointsCache();

dito

This sound sane to me.
Ideally we would store only the string in case of an unknown fs or the enum but not both for a same entry, the choice could be done in ctor.

Just leave a leave a comment as to why we want to keep those.

Detaches m_mtabCache, just qAsConst or temporary const variable

I have added a new enum class type FilesystemType to represent the known filesystem types. Currently I have just added a single known filesystem type, but this should be extended over time. I have also added the necessary from/to string conversion functions.

I have wrapped all lines to fit 80 characters in width

Good point :)
Actually, I don't know why I called the function mountPath instead of mountPoint - but I have reamed it, along with the member etc.

The code didn't need the for loop anyway, so I have rewritten it

The code didn't need the for loop anyway, so I have rewritten it

It was merely meant as an optimization, I don't know if it makes sense - I'll remove it.

Good point :) I have changed this as you suggested, but I have added a method to FilesystemEntry called isValid to check if it is valid

Yes definitely :) I have removed the line

Fixed

I have added handling of overlayfs and CryFs and removed the '@' separator. Actually I don't know why I added the '@', I was sure that I had seen it somewhere in the code, but I guess that is not the case ;)

I tried to do a solid-hardware5 list details and compare the before and after output, and it should be the same now. I tested with both encfs and cryfs filesystems.

I guess the main reason is that the id is a function of a FilesystemEntry, meaning that the id is computed from the contents of it. You are absolutely right that it is not picked up directly from the fstab/mtab file, but it is computed from the contents. I think it should either be a member function of the FilesystemEntry class or a static member function of some other class (taking the FilesystemEntry as an input paramter) - it at least has to be in a place where it is possible to write a unit test for it's functionality. especially since the id() is used as part of the public API. Therefore I don't think we should make this an internal function in fstabhandling. Another reason is that, we might need the id in other places then the fstabhandling, at least we need it for the rest of the fstab handling refactor I posted in the other review.

So long story short :)
I think we should either keep it here or make it a public (static) method somewhere else - @bruns what do you think?

For now just keep it here, I can see now benefit of making it public (even unexported).

And thanks for baring with my nitpicking ;-), this now is pretty much what I hoped for.

No problem, I really appreciate the thorough review feedback you provide. I definitely prefer to take more time and get things right, then to rush things and potentially get them wrong.

So, have you had a chance to test out the patch? Do you think it is ready to land?

I will also start to prepare the next patch as part of splitting up the mega-refactor patch from D26600

The temporary for fstype is gone it seems.

The id() method and members are still part of the FstabEntry, where they do not belong.

The UDIs go through some further mangling in FstabHandling::deviceList(), which should be covered by the unit-tests as well.

Conceptually, the _k_deviceNameForMountpoint/_k_deviceNameForFilesystemEntry should be part of the m_mtabCache/m_fstabCache class (which currently is just a QMultiHash<>). This class can then be unit tested easily.

For now, keep the _k_deviceNameForMountpoint inside fstabhandling.cpp

@bruns - I am little confused now. You wrote previously that you could see the point in the id() function, but now you want it moved?
What kind of unit test are you missing? I can't quite follow the added mangling in FstabHandling::deviceList()? I don't think we have (or ever have) had any unit test of FstabHandling, but that was one of the things I am trying to achieve was this patch series that breaks up and decouples the code pieces into smaller bits.

Could I ask you to take a look at it again? Sorry for nagging, I just really want to close this up soon and move on with the next patches ;D

I never said anything like that. "here" == fstabhandling.cpp

The name is exported by deviceList(), after mangling it. Thats the reason any further processing should go there, not into fstentry, but that is matter for another patch.

The code should be be split up, but the splitting should be done at the right places.