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

[Wolfgang Bumiller]

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 ;-)

>                     }
>                 }
>             }
>             true
>         };
> 
> > +                })
> > +            })
> > +        });
> > +        if !datastore_empty {
> > +            bail!("path not empty!");
> > +        }
> > +        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(),
> > +        )?;
> > +    }
> >  
> >      config.set_data(&datastore.name, "datastore", &datastore)?;
> >