Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Let callers handle errors #204

Open
simonsan opened this issue Mar 13, 2024 · 1 comment
Open

Let callers handle errors #204

simonsan opened this issue Mar 13, 2024 · 1 comment
Labels
A-errors Area: error handling needs improvement C-enhancement Category: New feature or request C-refactor Category: Refactoring of already existing code

Comments

@simonsan
Copy link
Contributor

If we encounter an error, we should return an error and let the caller handle it. In the following code example, we encounter errors, but instead of returning them as such, we are returning Ok(()) and logging an error. IMHO, logging it is fine, but we should also return an Error in case of encountering one. We should fix that all over the code base.

    // check header
    let header = be.decrypt(&data.split_off(data.len() - header_len as usize))?;

    let pack_blobs = PackHeader::from_binary(&header)?.into_blobs();
    let mut blobs = index_pack.blobs;
    blobs.sort_unstable_by_key(|b| b.offset);
    if pack_blobs != blobs {
        error!("pack {id}: Header from pack file does not match the index");
        debug!("pack file header: {pack_blobs:?}");
        debug!("index: {:?}", blobs);
        return Ok(());
    }
    p.inc(u64::from(header_len) + 4);

    // check blobs
    for blob in blobs {
        let blob_id = blob.id;
        let mut blob_data = be.decrypt(&data.split_to(blob.length as usize))?;

        // TODO: this is identical to backend/decrypt.rs; unify these two parts!
        if let Some(length) = blob.uncompressed_length {
            blob_data = decode_all(&*blob_data)?;
            if blob_data.len() != length.get() as usize {
                error!("pack {id}, blob {blob_id}: Actual uncompressed length does not fit saved uncompressed length");
                return Ok(());
            }
        }

        let comp_id = hash(&blob_data);
        if blob.id != comp_id {
            error!("pack {id}, blob {blob_id}: Hash mismatch. Computed hash: {comp_id}");
            return Ok(());
        }
        p.inc(blob.length.into());

src.: check header

@simonsan simonsan added A-errors Area: error handling needs improvement C-enhancement Category: New feature or request C-refactor Category: Refactoring of already existing code labels Mar 13, 2024
@github-actions github-actions bot added the S-triage Status: Waiting for a maintainer to triage this issue/PR label Mar 13, 2024
@simonsan simonsan removed the S-triage Status: Waiting for a maintainer to triage this issue/PR label Mar 14, 2024
@simonsan
Copy link
Contributor Author

simonsan commented Mar 14, 2024

other example:

    fn set_metadata(self, dest: &LocalDestination, path: &PathBuf, node: &Node) {
        debug!("setting metadata for {:?}", path);
        dest.create_special(path, node)
            .unwrap_or_else(|_| warn!("restore {:?}: creating special file failed.", path));
        match (self.no_ownership, self.numeric_id) {
            (true, _) => {}
            (false, true) => dest
                .set_uid_gid(path, &node.meta)
                .unwrap_or_else(|_| warn!("restore {:?}: setting UID/GID failed.", path)),
            (false, false) => dest
                .set_user_group(path, &node.meta)
                .unwrap_or_else(|_| warn!("restore {:?}: setting User/Group failed.", path)),
        }
        dest.set_permission(path, node)
            .unwrap_or_else(|_| warn!("restore {:?}: chmod failed.", path));
        dest.set_extended_attributes(path, &node.meta.extended_attributes)
            .unwrap_or_else(|_| warn!("restore {:?}: setting extended attributes failed.", path));
        dest.set_times(path, &node.meta)
            .unwrap_or_else(|_| warn!("restore {:?}: setting file times failed.", path));
    }

fn set_metadata(self, dest: &LocalDestination, path: &PathBuf, node: &Node) {

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-errors Area: error handling needs improvement C-enhancement Category: New feature or request C-refactor Category: Refactoring of already existing code
Projects
None yet
Development

No branches or pull requests

1 participant