Add `new_raw` IppRequest constructor, `DeleteAttr` IppValueTag, and standard attribute helper by bakayu · Pull Request #10 · OpenPrinting/cups-rs

bakayu · 2026-02-07T18:27:00Z

Summary

This PR extends the IppRequest and IppValueTag APIs to support administrative operations (like CUPS-Add-Modify-Printer and CUPS-Get-Devices) without forcing downstream applications to use unsafe blocks or raw FFI.

Changes:

Added IppRequest::new_raw(op_code: i32):
- Allows creating an IPP request with an integer operation code.
- Motivation: Essential for supporting operations that are either deprecated, but might be required to handle legacy operations (like CUPS_GET_DEVICES 0x400B) or not yet part of the IppOperation enum.
Added IppValueTag::DeleteAttr:
- Maps to the IPP tag IPP_TAG_DELETEATTR (0x13).
- Motivation: Required for CUPS-Add-Modify-Printer operations where specific attributes need to be unset/deleted.
Added IppRequest::add_standard_attrs():
- Helper to automatically inject standard IPP attributes (attributes-charset, attributes-natural-language, requesting-user-name).
- Motivation: Reduces boilerplate code for every IPP request.

Use Case / Context

These additions were discovered while porting lpinfo and lpadmin to Rust. Previously, implementing lpinfo -v (Device Discovery) and lpadmin (Queue Management) required significant usage of unsafe blocks in the application code to access these missing tags and operations. These changes allow those tools to be written safely using the library's abstractions.

Previous oxidized version of lpinfo: https://github.com/bakayu/lpinfo-rs/blob/master/src/main.rs
New oxidized version of lpinfo (no usage of unsafe blocks): https://github.com/bakayu/lpinfo-rs/blob/feat-use-safe-wrappers/src/main.rs
Previous oxidized version of lpadmin: https://github.com/bakayu/lpadmin-rs/blob/master/src/ipp_helpers.rs
New oxidized version of lpadmin (no usage of unsafe blocks): https://github.com/bakayu/lpadmin-rs/blob/feat-use-safe-wrappers/src/ipp_helpers.rs

Abd002 · 2026-02-09T16:55:31Z

I recommend you to rebase commits and squash last one.

Gmin2 · 2026-02-11T06:40:13Z

Hey little occupied for now!, will glance through this in the weekend

bakayu · 2026-02-16T06:20:40Z

Hi @Gmin2
Sorry for the ping, but are there any updates on this?

bakayu · 2026-03-05T10:11:48Z

Hi @Gmin2 Sorry for the ping, but are there any updates on this?

re: @Gmin2

Gmin2 · 2026-03-09T11:36:29Z

nice works, looks good with the first pass!

Copilot

Pull request overview

This PR expands the low-level IPP request builder API to better support administrative / CUPS-specific operations without requiring downstream crates to use unsafe FFI directly.

Changes:

Added IppRequest::new_raw(op_code: i32) to construct requests from raw/legacy operation codes.
Added IppValueTag::DeleteAttr mapping for IPP_TAG_DELETEATTR.
Added IppRequest::add_standard_attrs() helper and a basic unit test for it.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-03-09T11:41:06Z

src/ipp.rs

+    /// 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,
+        })
+    }


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

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

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

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.

src/ipp.rs

Copilot · 2026-03-09T11:41:07Z

src/ipp.rs

+        let result = request.add_standard_attrs();
+        assert!(result.is_ok());


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"

);

}

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

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

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

src/ipp.rs

Copilot · 2026-03-09T11:41:07Z

src/ipp.rs

    Uri,
    Charset,
    Language,
    MimeType,
+    DeleteAttr,
 }


IppValueTag::DeleteAttr is an out-of-band IPP value tag (used to delete/unset an attribute) and generally should be sent without a string/integer payload. The current safe API only exposes add_string/add_integer/add_boolean/add_strings, so callers still have no safe way to construct a delete-attr operation without passing a dummy value or dropping to raw bindings. Consider adding a dedicated helper (e.g., add_out_of_band(...) or delete_attr(name, group)) that calls the appropriate CUPS IPP function for out-of-band values, and document that DeleteAttr is only valid through that helper.

DeleteAttr can be added via add_integer, here is an example for a helper fn which I used in my rust port of lpadmin:

fn add_delete_attr( request: &mut IppRequest, option: &str, ) -> Result<(), Box<dyn std::error::Error>> { request.add_integer(IppTag::Printer, IppValueTag::DeleteAttr, option, 0)?; Ok(()) }

…ndard attributes Signed-off-by: Ayush <mail@ayuch.dev> feat: add `DeleteAttr` IppValueTag Signed-off-by: Ayush <mail@ayuch.dev> refacator: use `config::get_user();` instead of reading `USER` env var Signed-off-by: Ayush <mail@ayuch.dev> test: add test for `new_raw` IppRequest constructor Signed-off-by: Ayush <mail@ayuch.dev>

bakayu force-pushed the extend-wrappers branch from ae3c6fc to 5d0972b Compare February 10, 2026 18:13

Gmin2 mentioned this pull request Feb 11, 2026

Fix IPP request op handling; add Destination attr fetch convenience API #9

Open

Gmin2 requested a review from Copilot March 9, 2026 11:36

Copilot started reviewing on behalf of Gmin2 March 9, 2026 11:37 View session

Copilot AI reviewed Mar 9, 2026

View reviewed changes

bakayu force-pushed the extend-wrappers branch from 5d0972b to 964873d Compare March 9, 2026 12:49

bakayu force-pushed the extend-wrappers branch from 964873d to 6af0ac1 Compare March 9, 2026 12:53

		let result = request.add_standard_attrs();
		assert!(result.is_ok());

-        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"
+            );
+        }

Conversation

bakayu commented Feb 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Changes:

Use Case / Context

Uh oh!

Abd002 commented Feb 9, 2026

Uh oh!

Gmin2 commented Feb 11, 2026

Uh oh!

bakayu commented Feb 16, 2026

Uh oh!

bakayu commented Mar 5, 2026

Uh oh!

Gmin2 commented Mar 9, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Mar 9, 2026

Choose a reason for hiding this comment

Uh oh!

bakayu Mar 9, 2026

Choose a reason for hiding this comment

Uh oh!

Gmin2 Mar 11, 2026

Choose a reason for hiding this comment

Uh oh!

bakayu Mar 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI Mar 9, 2026

Choose a reason for hiding this comment

Uh oh!

bakayu Mar 9, 2026

Choose a reason for hiding this comment

Uh oh!

Gmin2 Mar 11, 2026

Choose a reason for hiding this comment

Uh oh!

bakayu Mar 12, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI Mar 9, 2026

Choose a reason for hiding this comment

Uh oh!

bakayu Mar 9, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

bakayu commented Feb 7, 2026 •

edited

Loading

bakayu Mar 12, 2026 •

edited

Loading