D15843: Allow FileIndexerConfig to check device mounted status by path.

[Stefan Brüns]

bruns added a comment.


  In D15843#335011 , @smithjd wrote:
  
  > In D15843#334518 , @ngraham 
wrote:
  >
  > >
  >
  >
  > This patch (or the other) is a requirement to augment the **current 
implementation** indexing of multiple devices with **safe** cleanup of the dead 
entries of multiple devices. It does not care about potential problems with the 
generated document id used by the database.
  
  
  No, it is not safe. Did some experiments with 2 Vaults a few days ago (Vault 
does not matter, could be any other mounted storage). After unmounting both and 
mounting one again, it had the device ID of the other one.

REPOSITORY
  R293 Baloo

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

To: smithjd, #baloo, bruns
Cc: ngraham, kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, 
spoorun, bruns, abrahams

D15843: Allow FileIndexerConfig to check device mounted status by path.

[James Smith]

smithjd added a comment.


  In D15843#334518 , @ngraham wrote:
  
  > Perhaps we should discuss the implementation of multi-device indexing in a 
Phab ticket instead of across the comments of multiple patches. Then we can 
settle on an agreed-upon approach and go implement it instead of going in 
circles with different approaches.
  
  
  This patch (or the other) is a requirement to augment the **current 
implementation** indexing of multiple devices with **safe** cleanup of the dead 
entries of multiple devices. It does not care about potential problems with the 
generated document id used by the database.

REPOSITORY
  R293 Baloo

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

To: smithjd, #baloo, bruns
Cc: ngraham, kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, 
spoorun, bruns, abrahams

D15843: Allow FileIndexerConfig to check device mounted status by path.

[Nathaniel Graham]

ngraham added a comment.


  Perhaps we should discuss the implementation of multi-device indexing in a 
Phab ticket instead of across the comments of multiple patches. Then we can 
settle on an agreed-upon approach and go implement it instead of going in 
circles with different approaches.

REPOSITORY
  R293 Baloo

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

To: smithjd, #baloo, bruns
Cc: ngraham, kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, 
spoorun, bruns, abrahams

D15843: Allow FileIndexerConfig to check device mounted status by path.

[James Smith]

smithjd added a comment.


  >>> As a side note - cross-device indexing is broken anyway and can never 
work reliably in the current scheme using device ids as part of the baloo 
document id. Device ids are not stable. For the home partition it works 
somewhat reliable as the device setup is sufficiently fixed, but for any 
network share, Vault, disk image, whatever, it depends on the mounting order.
  >> 
  >> 
  >> 
  >>> I will keep opposing any changes trying to paper over it.
  >> 
  >> I have not had a problem with mounted external volumes. FYI, complain to 
the Solid people about the non-randomness of the udi. That isn't a fault of 
Baloo.
  > 
  > You clearly lack understanding of the problem. This is not about solid 
UDIs, but about device ids provided by the kernel (man 7 inode) . Complain to 
the Linux folks!?
  
  That isn't a problem I'm particularly concerned with, that observance being 
unrelated to this patch. Is there a bug filed?

REPOSITORY
  R293 Baloo

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

To: smithjd, #baloo, bruns
Cc: ngraham, kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, 
spoorun, bruns, abrahams

D15843: Allow FileIndexerConfig to check device mounted status by path.

[Stefan Brüns]

bruns added a comment.


  In D15843#334515 , @smithjd wrote:
  
  > >> There is no need for this, just create a new StorageDevices where you 
need it.
  > >> Creating a second StorageDevices instance in a process is quite cheap.
  > > 
  > > Creating a separate object cluttered the console with duplicated debug 
output and raised a threading error.
  >
  > No go.
  >
  > This patch is a little bit tidier than exporting the StorageDevices object. 
Both are sufficient for being able to verify that the path is mounted.
  
  
  You should not export it but create an instance ...
  
  >> As a side note - cross-device indexing is broken anyway and can never work 
reliably in the current scheme using device ids as part of the baloo document 
id. Device ids are not stable. For the home partition it works somewhat 
reliable as the device setup is sufficiently fixed, but for any network share, 
Vault, disk image, whatever, it depends on the mounting order.
  > 
  > 
  > 
  >> I will keep opposing any changes trying to paper over it.
  > 
  > I have not had a problem with mounted external volumes. FYI, complain to 
the Solid people about the non-randomness of the udi. That isn't a fault of 
Baloo.
  
  You clearly lack understanding of the problem. This is not about solid UDIs, 
but about device ids provided by the kernel (man 7 inode) . Complain to the 
Linux folks!?

REPOSITORY
  R293 Baloo

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

To: smithjd, #baloo, bruns
Cc: ngraham, kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, 
spoorun, bruns, abrahams

D15843: Allow FileIndexerConfig to check device mounted status by path.

[James Smith]

smithjd added a comment.


  >> There is no need for this, just create a new StorageDevices where you need 
it.
  >> Creating a second StorageDevices instance in a process is quite cheap.
  > 
  > Creating a separate object cluttered the console with duplicated debug 
output and raised a threading error.
  
  No go.
  
  This patch is a little bit tidier than exporting the StorageDevices object. 
Both are sufficient for being able to verify that the path is mounted.
  
  > As a side note - cross-device indexing is broken anyway and can never work 
reliably in the current scheme using device ids as part of the baloo document 
id. Device ids are not stable. For the home partition it works somewhat 
reliable as the device setup is sufficiently fixed, but for any network share, 
Vault, disk image, whatever, it depends on the mounting order.
  
  
  
  > I will keep opposing any changes trying to paper over it.
  
  I have not had a problem with mounted external volumes. FYI, complain to the 
Solid people about the non-randomness of the udi. That isn't a fault of Baloo.

REPOSITORY
  R293 Baloo

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

To: smithjd, #baloo, bruns
Cc: ngraham, kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, 
spoorun, bruns, abrahams

D15843: Allow FileIndexerConfig to check device mounted status by path.

[Stefan Brüns]

bruns added a comment.


  As a side note - cross-device indexing is broken anyway and can never work 
reliably in the current scheme using device ids as part of the baloo document 
id. Device ids are not stable. For the home partition it works somewhat 
reliable as the device setup is sufficiently fixed, but for any network share, 
Vault, disk image, whatever, it depends on the mounting order.
  
  I will keep opposing any changes trying to paper over it.

REPOSITORY
  R293 Baloo

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

To: smithjd, #baloo, bruns
Cc: ngraham, kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, 
spoorun, bruns, abrahams

D15843: Allow FileIndexerConfig to check device mounted status by path.

[Stefan Brüns]

bruns added a comment.


  To cite myself from the mentioned review:
  
  > There is no need for this, just create a new StorageDevices where you need 
it.
  >  Creating a second StorageDevices instance in a process is quite cheap.
  
  If you need the info in DatabaseSanitizer, create the StorageDevices there ...

REPOSITORY
  R293 Baloo

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

To: smithjd, #baloo, bruns
Cc: ngraham, kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, 
spoorun, bruns, abrahams

D15843: Allow FileIndexerConfig to check device mounted status by path.

[James Smith]

smithjd added a comment.


  In D15843#334230 , @bruns wrote:
  
  > Most obvious problem with this change - as far as I can deduce from your 
description, this is about runtime behaviour. The config class is the wrong 
place to add this method.
  
  
  This isn't that different from shouldBeIndexed and it's related methods. 
Anywhere else would require exporting the StorageDevices object, already 
actively opposed by you: https://phabricator.kde.org/D15047?
  
  Please provide a credible alternative if you're going to oppose new 
additions, or oppose new features and clearly explain your opposition if you 
intend to block them by opposing new additions.

REPOSITORY
  R293 Baloo

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

To: smithjd, #baloo, bruns
Cc: ngraham, kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, 
spoorun, bruns, abrahams

D15843: Allow FileIndexerConfig to check device mounted status by path.

[Stefan Brüns]

bruns requested changes to this revision.
bruns added a comment.
This revision now requires changes to proceed.


  Most obvious problem with this change - as far as I can deduce from your 
description, this is about runtime behaviour. The config class is the wrong 
place to add this method.

REPOSITORY
  R293 Baloo

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

To: smithjd, #baloo, bruns
Cc: ngraham, kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, 
spoorun, bruns, abrahams

D15843: Allow FileIndexerConfig to check device mounted status by path.

[Nathaniel Graham]

ngraham added a comment.


  I think what @bruns is gently trying to indicate is that your Summary needs 
improvement. :) Remember that it becomes a part of the commit message once the 
patch lands.
  
  A Test Plan wouldn't hurt, either.

REPOSITORY
  R293 Baloo

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

To: smithjd, #baloo, bruns
Cc: ngraham, kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, 
spoorun, bruns, abrahams

D15843: Allow FileIndexerConfig to check device mounted status by path.

[James Smith]

smithjd added a comment.


  In D15843#334024 , @bruns wrote:
  
  > Whats the use case exactly? "Some parts" is not sufficient ...
  
  
  The index cleaner for example needs to skip unmounted paths.

REPOSITORY
  R293 Baloo

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

To: smithjd, #baloo, bruns
Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams

D15843: Allow FileIndexerConfig to check device mounted status by path.

[Stefan Brüns]

bruns added a comment.


  Whats the use case exactly? "Some parts" is not sufficient ...

REPOSITORY
  R293 Baloo

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

To: smithjd, #baloo, bruns
Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams

D15843: Allow FileIndexerConfig to check device mounted status by path.

[Nathaniel Graham]

ngraham added reviewers: Baloo, bruns.

REPOSITORY
  R293 Baloo

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

To: smithjd, #baloo, bruns
Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams

D15843: Allow FileIndexerConfig to check device mounted status by path.

[James Smith]

smithjd created this revision.
Herald added projects: Frameworks, Baloo.
Herald added subscribers: Baloo, kde-frameworks-devel.
smithjd requested review of this revision.

REVISION SUMMARY
  Parts of Baloo need some way to check if a path's volume is mounted or not.

REPOSITORY
  R293 Baloo

BRANCH
  master-pathDeviceMounted (branched from master)

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

AFFECTED FILES
  src/file/fileindexerconfig.cpp
  src/file/fileindexerconfig.h

To: smithjd
Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams