Add support for excluding domains and locations in metalink parsing#357
Add support for excluding domains and locations in metalink parsing#357bc-lee wants to merge 1 commit intorpm-software-management:masterfrom
Conversation
This commit introduces two new options, LRO_METALINK_EXCLUDE_DOMAIN and LRO_METALINK_EXCLUDE_LOCATION, to the LrHandle structure. These options allow users to specify lists of domains and locations to exclude during metalink processing.
ppisar
left a comment
ppisar
left a comment
There was a problem hiding this comment.
While I welcome this feature, I think the interface (a scalar regular expression, not breaking ABI) and implementation (validate early) could be better.
| LRO_PASSWORD, /*!< (char *) | ||
| Password for HTTP authentication */ | ||
|
|
||
| LRO_METALINK_EXCLUDE_DOMAIN, /*!< (char ** NULL-terminated) |
There was a problem hiding this comment.
This enhances API. You need to increase a minor version of librepo in VERSION.cmake.
| Password for HTTP authentication */ | ||
|
|
||
| LRO_METALINK_EXCLUDE_DOMAIN, /*!< (char ** NULL-terminated) | ||
| List of domains to exclude from metalink */ |
There was a problem hiding this comment.
Also please append to the documentation since which version is this option available.
| } | ||
| } else { | ||
| // Invalid regex, treat as literal string | ||
| g_warning("%s: Invalid regex for metalink location exclusion \"%s\": %s", |
There was a problem hiding this comment.
Please spell "regex" in full as "regular expression". This is not Perl.
|
|
||
| gboolean | ||
| lr_metalink_parse_file(LrMetalink *metalink, | ||
| LrHandle *handle, |
There was a problem hiding this comment.
lr_metalink_parse_file() is public functio. This breaks API and ABI. I think we need to wind a different way of passing the exclusion options. If there is not context or object to store the options to, I recommend adding a new function and making this old as a single wrapper around it.
|
|
||
| .. data:: LRO_METALINK_EXCLUDE_DOMAIN | ||
|
|
||
| *List of strings*. List of regex patterns to exclude domains from metalink. |
There was a problem hiding this comment.
Why is it a list? If it is a regular expression, then string is enough.
What kind of regular expression it it? That needs to be documented.
| for (int i = 0; pd->handle->metalink_exclude_location[i]; i++) { | ||
| const char *pattern = pd->handle->metalink_exclude_location[i]; | ||
| GError *regex_err = NULL; | ||
| GRegex *regex = g_regex_new(pattern, |
There was a problem hiding this comment.
Why do you compile and validate the expression here deep in the XML parser? Best place is in lr_handle_setopt().
|
Thanks for the review. I think I need to redesign the API to reduce API/ABI incompatibilities. It will take a few days, so I’ll mark this PR as a draft for now. |
2 participants
This commit introduces two new options, LRO_METALINK_EXCLUDE_DOMAIN and LRO_METALINK_EXCLUDE_LOCATION, to the LrHandle structure. These options allow users to specify lists of domains and locations to exclude during metalink processing.
Related: rpm-software-management/dnf5#1483