Skip to content

Add new_raw IppRequest constructor, DeleteAttr IppValueTag, and standard attribute helper #10

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.

Sign up for GitHub

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

Open
bakayu wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Add new_raw IppRequest constructor, DeleteAttr IppValueTag, and standard attribute helper #10

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
93 changes: 77 additions & 16 deletions src/ipp.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,9 +30,9 @@
//! }
//! ```

use crate::bindings;
use crate::connection::HttpConnection;
use crate::error::{Error, Result};
use crate::{bindings, config};
use std::ffi::{CStr, CString};
use std::marker::PhantomData;
use std::ptr;
Expand Down Expand Up @@ -83,6 +83,7 @@ pub enum IppValueTag {
Charset,
Language,
MimeType,
DeleteAttr,
}

impl From<IppValueTag> for bindings::ipp_tag_t {
Expand All @@ -99,6 +100,7 @@ impl From<IppValueTag> for bindings::ipp_tag_t {
IppValueTag::Charset => bindings::ipp_tag_e_IPP_TAG_CHARSET,
IppValueTag::Language => bindings::ipp_tag_e_IPP_TAG_LANGUAGE,
IppValueTag::MimeType => bindings::ipp_tag_e_IPP_TAG_MIMETYPE,
IppValueTag::DeleteAttr => bindings::ipp_tag_e_IPP_TAG_DELETEATTR,
}
}
}
Expand Down Expand Up @@ -169,24 +171,18 @@ impl IppStatus {
bindings::ipp_status_e_IPP_STATUS_OK_IGNORED_OR_SUBSTITUTED => {
IppStatus::OkIgnoredOrSubstituted
}
bindings::ipp_status_e_IPP_STATUS_OK_CONFLICTING => {
IppStatus::OkConflicting
}
bindings::ipp_status_e_IPP_STATUS_OK_CONFLICTING => IppStatus::OkConflicting,
bindings::ipp_status_e_IPP_STATUS_ERROR_BAD_REQUEST => IppStatus::ErrorBadRequest,
bindings::ipp_status_e_IPP_STATUS_ERROR_FORBIDDEN => IppStatus::ErrorForbidden,
bindings::ipp_status_e_IPP_STATUS_ERROR_NOT_AUTHENTICATED => {
IppStatus::ErrorNotAuthenticated
}
bindings::ipp_status_e_IPP_STATUS_ERROR_NOT_AUTHORIZED => {
IppStatus::ErrorNotAuthorized
}
bindings::ipp_status_e_IPP_STATUS_ERROR_NOT_AUTHORIZED => IppStatus::ErrorNotAuthorized,
bindings::ipp_status_e_IPP_STATUS_ERROR_NOT_POSSIBLE => IppStatus::ErrorNotPossible,
bindings::ipp_status_e_IPP_STATUS_ERROR_TIMEOUT => IppStatus::ErrorTimeout,
bindings::ipp_status_e_IPP_STATUS_ERROR_NOT_FOUND => IppStatus::ErrorNotFound,
bindings::ipp_status_e_IPP_STATUS_ERROR_GONE => IppStatus::ErrorGone,
bindings::ipp_status_e_IPP_STATUS_ERROR_REQUEST_ENTITY => {
IppStatus::ErrorRequestEntity
}
bindings::ipp_status_e_IPP_STATUS_ERROR_REQUEST_ENTITY => IppStatus::ErrorRequestEntity,
bindings::ipp_status_e_IPP_STATUS_ERROR_REQUEST_VALUE => IppStatus::ErrorRequestValue,
bindings::ipp_status_e_IPP_STATUS_ERROR_DOCUMENT_FORMAT_NOT_SUPPORTED => {
IppStatus::ErrorDocumentFormatNotSupported
Expand Down Expand Up @@ -248,6 +244,23 @@ impl IppRequest {
})
}

/// Create a new IPP request from a raw operation code.
/// This is useful for deprecated operations.
pub fn new_raw(op_code: i32) -> Result<Self> {
let ipp = unsafe { bindings::ippNewRequest(op_code) };

if ipp.is_null() {
return Err(Error::UnsupportedFeature(
"Failed to create IPP request".to_string(),
));
}

Ok(IppRequest {
ipp,
_phantom: PhantomData,
})
}
Comment on lines +247 to +262
Copy link

Copilot AI Mar 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

new_raw duplicates the logic in IppRequest::new (same call + null check + struct construction). To keep the constructors consistent over time, consider refactoring to a shared private helper (or have new call into new_raw(operation.into() as i32) if the types line up).

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Author

@bakayu bakayu Mar 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This works in theory, would it be better to change new to this or keep both the constructors as they are currently? @Gmin2

impl IppRequest {
    pub fn new(operation: IppOperation) -> Result<Self> {
        Self::new_raw(operation as i32)
    }
}

Copy link
Copy Markdown
Collaborator

@Gmin2 Gmin2 Mar 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it will be better to remove the new_raw method, I dont see any utility of it !

Copy link
Copy Markdown
Author

@bakayu bakayu Mar 12, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, there is no long term utility, I intended to add this as a temporary method to create IPP requests for Ipp operations that are not supported yet. For example server side queue management, I explained the usecase with examples in the PR description. Once we have more API coverage with safe wrappers, this would loose its purpose.

let me know what you think.

Comment thread
bakayu marked this conversation as resolved.

/// Get the raw pointer to the ipp_t structure
pub fn as_ptr(&self) -> *mut bindings::_ipp_s {
self.ipp
Expand Down Expand Up @@ -320,7 +333,12 @@ impl IppRequest {
let name_c = CString::new(name)?;

let attr = unsafe {
bindings::ippAddBoolean(self.ipp, group.into(), name_c.as_ptr(), value as ::std::os::raw::c_char)
bindings::ippAddBoolean(
self.ipp,
group.into(),
name_c.as_ptr(),
value as ::std::os::raw::c_char,
)
};

if attr.is_null() {
Expand All @@ -347,7 +365,8 @@ impl IppRequest {
.map(|v| CString::new(*v).map_err(Error::from))
.collect::<Result<Vec<_>>>()?;

let values_ptrs: Vec<*const ::std::os::raw::c_char> = values_c.iter().map(|s| s.as_ptr()).collect();
let values_ptrs: Vec<*const ::std::os::raw::c_char> =
values_c.iter().map(|s| s.as_ptr()).collect();

let attr = unsafe {
bindings::ippAddStrings(
Expand All @@ -371,6 +390,35 @@ impl IppRequest {
}
}

/// Add standard IPP operation attributes:
/// - attributes-charset = "utf-8"
/// - attributes-natural-language = "en"
/// - requesting-user-name = $USER (or "unknown")
pub fn add_standard_attrs(&mut self) -> Result<()> {
let user = config::get_user();

Comment thread
bakayu marked this conversation as resolved.
self.add_string(
IppTag::Operation,
IppValueTag::Charset,
"attributes-charset",
"utf-8",
)?;
self.add_string(
IppTag::Operation,
IppValueTag::Language,
"attributes-natural-language",
"en",
)?;
self.add_string(
IppTag::Operation,
IppValueTag::Name,
"requesting-user-name",
&user,
)?;

Ok(())
}

/// Send this request and receive a response
pub fn send(&self, connection: &HttpConnection, resource: &str) -> Result<IppResponse> {
let resource_c = CString::new(resource)?;
Expand Down Expand Up @@ -464,11 +512,11 @@ impl IppResponse {
Err(_) => return None,
};

let group_tag = group.map(|g| g.into()).unwrap_or(bindings::ipp_tag_e_IPP_TAG_ZERO);
let group_tag = group
.map(|g| g.into())
.unwrap_or(bindings::ipp_tag_e_IPP_TAG_ZERO);

let attr = unsafe {
bindings::ippFindAttribute(self.ipp, name_c.as_ptr(), group_tag)
};
let attr = unsafe { bindings::ippFindAttribute(self.ipp, name_c.as_ptr(), group_tag) };

if attr.is_null() {
None
Expand Down Expand Up @@ -562,6 +610,12 @@ mod tests {
assert!(request.is_ok());
}

#[test]
fn test_ipp_request_creation_from_raw() {
let request = IppRequest::new_raw(bindings::ipp_op_e_IPP_OP_CUPS_SET_DEFAULT);
assert!(request.is_ok());
}

#[test]
fn test_ipp_add_string() {
let mut request = IppRequest::new(IppOperation::GetPrinterAttributes).unwrap();
Expand All @@ -588,6 +642,13 @@ mod tests {
assert!(result.is_ok());
}

#[test]
fn test_add_standard_attrs() {
let mut request = IppRequest::new(IppOperation::GetJobs).unwrap();
let result = request.add_standard_attrs();
assert!(result.is_ok());
Comment on lines +648 to +649
Copy link

Copilot AI Mar 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test only asserts that add_standard_attrs() returns Ok(()) but doesn’t verify that the three required attributes were actually added (or that their values are correct). Since IppRequest::as_ptr() is available, the test can use the raw IPP helpers (e.g., ippFindAttribute) to assert the presence/tag/value of attributes-charset, attributes-natural-language, and requesting-user-name.

Suggested change
let result = request.add_standard_attrs();
assert!(result.is_ok());
request.add_standard_attrs().expect("Failed to add standard attributes");
let ipp = request.as_ptr();
assert!(!ipp.is_null(), "Underlying IPP pointer should not be null");
unsafe {
// Verify that the three required standard attributes are present
let charset_name = CString::new("attributes-charset").unwrap();
let natural_name = CString::new("attributes-natural-language").unwrap();
let user_name = CString::new("requesting-user-name").unwrap();
let charset_attr = bindings::ippFindAttribute(
ipp,
charset_name.as_ptr(),
bindings::ipp_tag_t_IPP_TAG_ZERO,
);
let natural_attr = bindings::ippFindAttribute(
ipp,
natural_name.as_ptr(),
bindings::ipp_tag_t_IPP_TAG_ZERO,
);
let user_attr = bindings::ippFindAttribute(
ipp,
user_name.as_ptr(),
bindings::ipp_tag_t_IPP_TAG_ZERO,
);
assert!(
!charset_attr.is_null(),
"attributes-charset attribute should be present"
);
assert!(
!natural_attr.is_null(),
"attributes-natural-language attribute should be present"
);
assert!(
!user_attr.is_null(),
"requesting-user-name attribute should be present"
);
// Verify that the attributes have the expected IPP value tags
assert_eq!(
bindings::ippGetValueTag(charset_attr),
bindings::ipp_tag_t_IPP_TAG_CHARSET,
"attributes-charset should have CHARSET value tag"
);
assert_eq!(
bindings::ippGetValueTag(natural_attr),
bindings::ipp_tag_t_IPP_TAG_LANGUAGE,
"attributes-natural-language should have LANGUAGE value tag"
);
assert_eq!(
bindings::ippGetValueTag(user_attr),
bindings::ipp_tag_t_IPP_TAG_NAME,
"requesting-user-name should have NAME value tag"
);
}

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Author

@bakayu bakayu Mar 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Gmin2 I believe we currently lack safe wrappers to check if certain IppValueTags were injected currently, what approach should we take here?

Copy link
Copy Markdown
Collaborator

@Gmin2 Gmin2 Mar 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Its important that yu have a comment explaining why this is safe, whenever using unsafe code !

Copy link
Copy Markdown
Author

@bakayu bakayu Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand which part you are referring to...

}

#[test]
fn test_ipp_status() {
assert!(IppStatus::Ok.is_successful());
Expand Down