D27152: Introduce FilesystemEntry class

[Stefan Brüns]

bruns added inline comments.

INLINE COMMENTS

> hallas wrote in fstabhandling.cpp:374
> @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.

REPOSITORY
  R245 Solid

REVISION DETAIL
  https://phabricator.kde.org/D27152

To: hallas, #frameworks, bruns, meven
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D27152: Introduce FilesystemEntry class

[David Hallas]

hallas marked an inline comment as done.
hallas added inline comments.

INLINE COMMENTS

> bruns wrote in fstabhandling.cpp:374
> 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

REPOSITORY
  R245 Solid

REVISION DETAIL
  https://phabricator.kde.org/D27152

To: hallas, #frameworks, bruns, meven
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D27152: Introduce FilesystemEntry class

[Stefan Brüns]

bruns added inline comments.

INLINE COMMENTS

> hallas wrote in fstabhandling.cpp:374
> 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 `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

REPOSITORY
  R245 Solid

REVISION DETAIL
  https://phabricator.kde.org/D27152

To: hallas, #frameworks, bruns, meven
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D27152: Introduce FilesystemEntry class

[David Hallas]

hallas marked an inline comment as done.
hallas added a comment.


  Hi @bruns  - did you have a chance to go through this patch again? Am I 
missing anything to move on with this?

REPOSITORY
  R245 Solid

REVISION DETAIL
  https://phabricator.kde.org/D27152

To: hallas, #frameworks, bruns, meven
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D27152: Introduce FilesystemEntry class

[Méven Car]

meven added inline comments.

INLINE COMMENTS

> bruns wrote in fstabhandling.cpp:209
> add temporary for fstype

The temporary for fstype is gone it seems.

REPOSITORY
  R245 Solid

REVISION DETAIL
  https://phabricator.kde.org/D27152

To: hallas, #frameworks, bruns, meven
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D27152: Introduce FilesystemEntry class

[David Hallas]

hallas marked 4 inline comments as done.
hallas added inline comments.

INLINE COMMENTS

> bruns wrote in fstabhandling.cpp:374
> 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 

REPOSITORY
  R245 Solid

REVISION DETAIL
  https://phabricator.kde.org/D27152

To: hallas, #frameworks, bruns, meven
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D27152: Introduce FilesystemEntry class

[Stefan Brüns]

bruns added inline comments.

INLINE COMMENTS

> hallas wrote in fstabhandling.cpp:374
> 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.

REPOSITORY
  R245 Solid

REVISION DETAIL
  https://phabricator.kde.org/D27152

To: hallas, #frameworks, bruns, meven
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D27152: Introduce FilesystemEntry class

[David Hallas]

hallas added inline comments.

INLINE COMMENTS

> bruns wrote in fstabhandling.cpp:374
> Why is the id() a property of the entry now?
> 
> - it does not correspond to anything from the fstab/mtab
> - it is only used here to generate the key for the cache
> 
> Please move the id generation back here, eventually changing it from 
> `_k_deviceNameForMountpoint(const QString , const QString 
> ,const QString )` to `_k_deviceIdForFsentry(const 
> FilesystemEntry&)`

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?

REPOSITORY
  R245 Solid

REVISION DETAIL
  https://phabricator.kde.org/D27152

To: hallas, #frameworks, bruns, meven
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D27152: Introduce FilesystemEntry class

[Stefan Brüns]

bruns requested changes to this revision.
bruns added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> fstabhandling.cpp:374
> +const FilesystemEntry fsEntry(fsname, mountpoint, type, 
> QStringList());
> +globalFstabCache->localData().m_mtabCache.insert(fsEntry.id(), 
> fsEntry);
>  }

Why is the id() a property of the entry now?

- it does not correspond to anything from the fstab/mtab
- it is only used here to generate the key for the cache

Please move the id generation back here, eventually changing it from 
`_k_deviceNameForMountpoint(const QString , const QString ,const 
QString )` to `_k_deviceIdForFsentry(const FilesystemEntry&)`

REPOSITORY
  R245 Solid

REVISION DETAIL
  https://phabricator.kde.org/D27152

To: hallas, #frameworks, bruns, meven
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D27152: Introduce FilesystemEntry class

[David Hallas]

hallas updated this revision to Diff 76990.
hallas marked 7 inline comments as done.
hallas added a comment.


  Review comments

REPOSITORY
  R245 Solid

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D27152?vs=76898=76990

BRANCH
  introduce_filesystem_entry

REVISION DETAIL
  https://phabricator.kde.org/D27152

AFFECTED FILES
  autotests/CMakeLists.txt
  autotests/filesystem_entry_test.cpp
  autotests/filesystem_entry_test.h
  autotests/filesystem_type_test.cpp
  autotests/filesystem_type_test.h
  src/solid/devices/backends/fstab/CMakeLists.txt
  src/solid/devices/backends/fstab/filesystem_entry.cpp
  src/solid/devices/backends/fstab/filesystem_entry.h
  src/solid/devices/backends/fstab/filesystem_type.cpp
  src/solid/devices/backends/fstab/filesystem_type.h
  src/solid/devices/backends/fstab/fstabdevice.cpp
  src/solid/devices/backends/fstab/fstabhandling.cpp
  src/solid/devices/backends/fstab/fstabhandling.h

To: hallas, #frameworks, bruns, meven
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D27152: Introduce FilesystemEntry class

[David Hallas]

hallas added inline comments.

INLINE COMMENTS

> bruns wrote in filesystem_entry.cpp:31
> This is a behavioral change:
> 
> - overlayfs is no longer handled
> - fuse.cryfs is no longer handled
> 
> - encfs gets an extra '@', and the prefix is changed from  to 
> 
> The identifier is API, don't change it. E.g. dolphin may reference it.

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.

REPOSITORY
  R245 Solid

REVISION DETAIL
  https://phabricator.kde.org/D27152

To: hallas, #frameworks, bruns, meven
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D27152: Introduce FilesystemEntry class

[Stefan Brüns]

bruns requested changes to this revision.
bruns added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> filesystem_entry_test.h:6
> + *   it under the terms of the GNU General Public License as published by  *
> + *   the Free Software Foundation; either version 2 of the License, or *
> + *   (at your option) any later version.   *

For tests LGPL is preferred as well. Also see D27742 
.

> filesystem_entry.cpp:31
> +case Solid::Backends::Fstab::FilesystemType::FuseEncFs:
> +return device + QLatin1Char('@') + mountPoint;
> +case Solid::Backends::Fstab::FilesystemType::Unknown:

This is a behavioral change:

- overlayfs is no longer handled
- fuse.cryfs is no longer handled

- encfs gets an extra '@', and the prefix is changed from  to 

The identifier is API, don't change it. E.g. dolphin may reference it.

> filesystem_entry.h:5
> + *   This program is free software; you can redistribute it and/or modify  *
> + *   it under the terms of the GNU General Public License as published by  *
> + *   the Free Software Foundation; either version 2 of the License, or *

This has to be LGPL.

Regarding wording, please have a look at D27742 
 for how it should look like.

Applies to other files as well.

REPOSITORY
  R245 Solid

REVISION DETAIL
  https://phabricator.kde.org/D27152

To: hallas, #frameworks, bruns, meven
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D27152: Introduce FilesystemEntry class

[David Hallas]

hallas updated this revision to Diff 76898.
hallas marked 9 inline comments as done.
hallas added a comment.


  Addressed review comments

REPOSITORY
  R245 Solid

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D27152?vs=75961=76898

BRANCH
  introduce_filesystem_entry

REVISION DETAIL
  https://phabricator.kde.org/D27152

AFFECTED FILES
  autotests/CMakeLists.txt
  autotests/filesystem_entry_test.cpp
  autotests/filesystem_entry_test.h
  autotests/filesystem_type_test.cpp
  autotests/filesystem_type_test.h
  src/solid/devices/backends/fstab/CMakeLists.txt
  src/solid/devices/backends/fstab/filesystem_entry.cpp
  src/solid/devices/backends/fstab/filesystem_entry.h
  src/solid/devices/backends/fstab/filesystem_type.cpp
  src/solid/devices/backends/fstab/filesystem_type.h
  src/solid/devices/backends/fstab/fstabdevice.cpp
  src/solid/devices/backends/fstab/fstabhandling.cpp
  src/solid/devices/backends/fstab/fstabhandling.h

To: hallas, #frameworks, bruns, meven
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D27152: Introduce FilesystemEntry class

[David Hallas]

hallas added a comment.


  @bruns  - thanks a lot for the feedback, I have updated the patch with your 
suggestions.

INLINE COMMENTS

> bruns wrote in filesystem_entry.cpp:77
> Why don't you just keep the string?

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

> bruns wrote in fstabdevice.cpp:41
> Ircs, raw pointers ... you newer know who owns them ... It is a pointer to an 
> entry in a QHash, it may be deleted/moved when you don't expect it.
> 
> Unfortunately, std::optional is C++17 only.
> 
> Return it by value, return a default constructed instance if no found, and 
> compare `fsEntry.device() != m_device`

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

> bruns wrote in fstabhandling.cpp:264
> this looks wrong ...

Yes definitely :) I have removed the line

> bruns wrote in fstabhandling.cpp:316
> this is bad, bad, bad, ...

Fixed

REPOSITORY
  R245 Solid

REVISION DETAIL
  https://phabricator.kde.org/D27152

To: hallas, #frameworks, bruns, meven
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D27152: Introduce FilesystemEntry class

[Stefan Brüns]

bruns requested changes to this revision.
bruns added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> filesystem_entry.cpp:77
> +{
> +return m_type == FilesystemType::Unknown ? m_typeString : 
> FilesystemTypeToString(m_type);
> +}

Why don't you just keep the string?

> fstabdevice.cpp:41
>  m_device.remove(parentUdi() + "/");
> +const FilesystemEntry* fsEntry = 
> FstabHandling::filesystemEntry(m_device);
> +if (fsEntry == nullptr) {

Ircs, raw pointers ... you newer know who owns them ... It is a pointer to an 
entry in a QHash, it may be deleted/moved when you don't expect it.

Unfortunately, std::optional is C++17 only.

Return it by value, return a default constructed instance if no found, and 
compare `fsEntry.device() != m_device`

> fstabhandling.cpp:264
> +}
> +return fstabEntry->mountOptions();
> +}

this looks wrong ...

> fstabhandling.cpp:316
> +if (fstabEntry != globalFstabCache->localData().m_fstabCache.end()) {
> +return &(*fstabEntry);
> +}

this is bad, bad, bad, ...

REPOSITORY
  R245 Solid

REVISION DETAIL
  https://phabricator.kde.org/D27152

To: hallas, #frameworks, bruns, meven
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D27152: Introduce FilesystemEntry class

[David Hallas]

hallas added a comment.


  Hi @bruns  - I have updated this patch with the changes you requested and 
would really like your feedback :)

REPOSITORY
  R245 Solid

REVISION DETAIL
  https://phabricator.kde.org/D27152

To: hallas, #frameworks, bruns, meven
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D27152: Introduce FilesystemEntry class

[Méven Car]

meven accepted this revision.

REPOSITORY
  R245 Solid

REVISION DETAIL
  https://phabricator.kde.org/D27152

To: hallas, #frameworks, bruns, meven
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D27152: Introduce FilesystemEntry class

[David Hallas]

hallas added inline comments.

INLINE COMMENTS

> meven wrote in filesystem_entry.cpp:47
> 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.

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.

> bruns wrote in filesystem_entry.h:34
> wrap long lines, also below.

I have wrapped all lines to fit 80 characters in width

> bruns wrote in filesystem_entry.h:48
> Whats wrong with mountPoint? Its used everywhere else here, and is a well 
> known term.

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.

> bruns wrote in fstabhandling.cpp:259
> detaches m_fstabCache, also below.

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

> meven wrote in fstabhandling.cpp:388
> Detaches m_mtabCache, just qAsConst or temporary const variable

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

REPOSITORY
  R245 Solid

REVISION DETAIL
  https://phabricator.kde.org/D27152

To: hallas, #frameworks, bruns, meven
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D27152: Introduce FilesystemEntry class

[David Hallas]

hallas updated this revision to Diff 75961.
hallas marked 17 inline comments as done.
hallas added a comment.


  Updated patch with review comments

REPOSITORY
  R245 Solid

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D27152?vs=74977=75961

BRANCH
  introduce_filesystem_entry

REVISION DETAIL
  https://phabricator.kde.org/D27152

AFFECTED FILES
  autotests/CMakeLists.txt
  autotests/filesystem_entry_test.cpp
  autotests/filesystem_entry_test.h
  autotests/filesystem_type_test.cpp
  autotests/filesystem_type_test.h
  src/solid/devices/backends/fstab/CMakeLists.txt
  src/solid/devices/backends/fstab/filesystem_entry.cpp
  src/solid/devices/backends/fstab/filesystem_entry.h
  src/solid/devices/backends/fstab/filesystem_type.cpp
  src/solid/devices/backends/fstab/filesystem_type.h
  src/solid/devices/backends/fstab/fstabdevice.cpp
  src/solid/devices/backends/fstab/fstabhandling.cpp
  src/solid/devices/backends/fstab/fstabhandling.h

To: hallas, #frameworks, bruns, meven
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D27152: Introduce FilesystemEntry class

[Méven Car]

meven added inline comments.

INLINE COMMENTS

> hallas wrote in filesystem_entry.cpp:47
> 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?

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.

> hallas wrote in filesystem_entry.h:88
> 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?

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

> fstabhandling.cpp:388
> +QStringList mountpoints;
> +for (const auto& dev : globalFstabCache->localData().m_mtabCache) {
> +if (!dev.mountPath().isEmpty()) {

Detaches m_mtabCache, just qAsConst or temporary const variable

REPOSITORY
  R245 Solid

REVISION DETAIL
  https://phabricator.kde.org/D27152

To: hallas, #frameworks, bruns, meven
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D27152: Introduce FilesystemEntry class

[Stefan Brüns]

bruns added inline comments.

INLINE COMMENTS

> CMakeLists.txt:3
> +filesystem_entry.cpp
> +filesystem_entry.h
>  fstabmanager.cpp

remove

> filesystem_entry.cpp:59
> +{
> +if (m_type == QLatin1Literal("fuse.encfs")) {
> +return m_device + QLatin1Char('@') + m_mountPath;

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

> filesystem_entry.h:34
> +/*
> + * Class that represents a filesystem. The filesystem is either mounted by 
> the system or can be mounted by the system.
> + * The information in the class is modelled around the fstab/mtab type files.

wrap long lines, also below.

> filesystem_entry.h:48
> + */
> +QString mountPath() const;
> +/*

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

> fstabhandling.cpp:168
>  const QString device = _k_deviceNameForMountpoint(fsname, 
> fstype, mountpoint);
>  QStringList options = 
> QFile::decodeName(fe->mnt_opts).split(QLatin1Char(','));
>  

should be const now

> fstabhandling.cpp:209
>  const QString mountpoint = items.at(1);
>  
> +globalFstabCache->localData().m_fstabCache.insert(device, 
> FilesystemEntry(device, mountpoint, items.at(2), QStringList()));

add temporary for fstype

> fstabhandling.cpp:259
> +QStringList mountpoints;
> +for (const auto& dev : globalFstabCache->localData().m_fstabCache) {
> +if (!dev.mountPath().isEmpty()) {

detaches m_fstabCache, also below.

> fstabhandling.cpp:275
>  {
>  _k_updateFstabMountPointsCache();
>  

missing `_k_updateMtabMountPointsCache();`

> fstabhandling.cpp:290
>  {
>  _k_updateFstabMountPointsCache();
>  

dito

REPOSITORY
  R245 Solid

REVISION DETAIL
  https://phabricator.kde.org/D27152

To: hallas, #frameworks, bruns, meven
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D27152: Introduce FilesystemEntry class

[Stefan Brüns]

bruns requested changes to this revision.
This revision now requires changes to proceed.

REPOSITORY
  R245 Solid

REVISION DETAIL
  https://phabricator.kde.org/D27152

To: hallas, #frameworks, bruns, meven
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D27152: Introduce FilesystemEntry class

[David Hallas]

hallas added inline comments.

INLINE COMMENTS

> meven wrote in filesystem_entry.cpp:47
> It would be great to expose an enum (KIO's KFileSystemType does this a 
> little).
> But given the number of filesystems this days, it may be overcomplicated.

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?

> meven wrote in filesystem_entry.h:88
> Is it necessary to keep if it is  commented out ?

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?

REPOSITORY
  R245 Solid

BRANCH
  introduce_filesystem_entry

REVISION DETAIL
  https://phabricator.kde.org/D27152

To: hallas, #frameworks, bruns, meven
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D27152: Introduce FilesystemEntry class

[Méven Car]

meven accepted this revision.
meven added a comment.
This revision is now accepted and ready to land.


  Nice one. Good step splitting out this.

INLINE COMMENTS

> filesystem_entry.cpp:47
> +
> +QString FilesystemEntry::type() const
> +{

It would be great to expose an enum (KIO's KFileSystemType does this a little).
But given the number of filesystems this days, it may be overcomplicated.

> filesystem_entry.h:88
> + */
> +//int m_dumpFrequency;
> +/*

Is it necessary to keep if it is  commented out ?

> filesystem_entry.h:94
> + */
> +//int m_passNo;
> +};

Same

REPOSITORY
  R245 Solid

BRANCH
  introduce_filesystem_entry

REVISION DETAIL
  https://phabricator.kde.org/D27152

To: hallas, #frameworks, bruns, meven
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D27152: Introduce FilesystemEntry class

[David Hallas]

hallas created this revision.
hallas added reviewers: Frameworks, bruns, meven.
hallas added a project: Frameworks.
hallas requested review of this revision.

REVISION SUMMARY
  Introdue a dedicated type for representing a FilesystemEntry. A
  FilesystemEntry is either mounted by the system or can be mounted by the
  system. It is created from the information found in filesystem table
  files, this can be fstab, mtab or any other mtab type file.
  
  This commit is a small part of a larger fstab refactor patch which can
  bee seen here D26600 

TEST PLAN
  Unit tests
  Tested manually with Plasma Vaults

REPOSITORY
  R245 Solid

BRANCH
  introduce_filesystem_entry

REVISION DETAIL
  https://phabricator.kde.org/D27152

AFFECTED FILES
  autotests/CMakeLists.txt
  autotests/filesystem_entry_test.cpp
  autotests/filesystem_entry_test.h
  src/solid/devices/backends/fstab/CMakeLists.txt
  src/solid/devices/backends/fstab/filesystem_entry.cpp
  src/solid/devices/backends/fstab/filesystem_entry.h
  src/solid/devices/backends/fstab/fstabhandling.cpp
  src/solid/devices/backends/fstab/fstabhandling.h

To: hallas, #frameworks, bruns, meven
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns