[pbs-devel] [PATCH proxmox-backup v5 1/2] fix #5439: allow to reuse existing datastore

[Gabriel Goller]

On 29.08.2024 13:58, Wolfgang Bumiller wrote:
>On Thu, Aug 29, 2024 at 01:56:46PM GMT, Wolfgang Bumiller wrote:
>> Just noticed another thing...
>>
>> On Thu, Aug 29, 2024 at 12:45:10PM GMT, Gabriel Goller wrote:
>> > @@ -70,21 +70,43 @@ pub(crate) fn do_create_datastore(
>> >      _lock: BackupLockGuard,
>> >      mut config: SectionConfigData,
>> >      datastore: DataStoreConfig,
>> > +    reuse_datastore: bool,
>> >  ) -> Result<(), Error> {
>> >      let path: PathBuf = datastore.path.clone().into();
>> >
>> > +    if path.parent().is_none() {
>> > +        bail!("cannot create datastore in root path");
>> > +    }
>> > +
>> >      let tuning: DatastoreTuning = serde_json::from_value(
>> >          DatastoreTuning::API_SCHEMA
>> >              .parse_property_string(datastore.tuning.as_deref().unwrap_or(""))?,
>> >      )?;
>> > -    let backup_user = pbs_config::backup_user()?;
>> > -    let _store = ChunkStore::create(
>> > -        &datastore.name,
>> > -        path,
>> > -        backup_user.uid,
>> > -        backup_user.gid,
>> > -        tuning.sync_level.unwrap_or_default(),
>> > -    )?;
>> > +
>> > +    if reuse_datastore {
>> > +        ChunkStore::verify_chunkstore(&path)?;
>> > +    } else {
>> > +        let datastore_empty = std::fs::read_dir(path.clone()).map_or(true, |mut d| {
>>
>> No need to clone() path here.
>>
>> > +            d.all(|dir| {
>> > +                dir.map_or(false, |file| {
>> > +                    file.file_name()
>> > +                        .to_str()
>> > +                        .map_or(false, |name| name.starts_with('.'))
>>
>> IMO if we manage to open the directory, but the iteration fails, we
>> should actually fail here as well.
>> Also we should not just ignore *anything* starting with a `.`, but
>> specifically only exactly `.` and `..`.
>>
>> Something like
>>
>>         let datastore_empty = 'empty: {
>>             if let Ok(dir) = std::fs::read_dir(&path) {
>>                 for file in dir {
>>                     let name = file?.file_name();
>>                     let name = name.as_encoded_bytes();
>>                     if name != b"." && name != b".." {
>>                         break 'empty false;
>
>Ummm
>Actually the `bail!()` could just be moved up here and the labeled block
>skipped, duh ;-)

Done!
Also reverted to `starts_with('.')` as discussed offlist to accomodate
for files such as '.zfs'.

Thanks for reviewing!
Posted a v6!