Re: [pve-devel] [PATCH v1 pve-storage 1/8] pluginbase: introduce PVE::Storage::PluginBase with doc scaffold

["Fabian Grünbichler" <f.gruenbichler@proxmox.com>]

On April 2, 2025 6:31 pm, Max Carrara wrote:
> On Mon Mar 31, 2025 at 5:13 PM CEST, Fabian Grünbichler wrote:
>> On March 26, 2025 3:20 pm, Max Carrara wrote:
>> > +=head2 HOOKS
>> > +
>> > +=cut
>> > +
>> > +# called during addition of storage (before the new storage config got written)
>>
>> called when adding a storage config entry, before the new config gets written
>>
>> > +# die to abort addition if there are (grave) problems
>> > +# NOTE: runs in a storage config *locked* context
>> > +sub on_add_hook {
>> > +    my ($class, $storeid, $scfg, %param) = @_;
>> > +    return undef;
>> > +}
>> > +
>> > +# called during storage configuration update (before the updated storage config got written)
>>
>> called when updating a storage config entry, before the updated config
>> gets written
>>
>> > +# die to abort the update if there are (grave) problems
>> > +# NOTE: runs in a storage config *locked* context
>> > +sub on_update_hook {
>> > +    my ($class, $storeid, $scfg, %param) = @_;
>> > +    return undef;
>> > +}
>> > +
>> > +# called during deletion of storage (before the new storage config got written)
>> > +# and if the activate check on addition fails, to cleanup all storage traces
>> > +# which on_add_hook may have created.
>>
>> called when deleting a storage config entry, before the new storage
>> config gets written.
>>
>> also called as part of error handling when undoing the addition of a new
>> storage config entry.
> 
> Regarding your three responses above: The comments here were preserved
> from `::Plugin` for context's sake. But tbh, on second thought, they can
> probably just be removed, as they'll be replaced by POD anyways.
> 

yes, but those parts mor or less made it to the POD as well slightly
rephrased, hence I called them out here already :-P

>>
>> > +# die to abort deletion if there are (very grave) problems
>> > +# NOTE: runs in a storage config *locked* context
>> > +sub on_delete_hook {
>> > +    my ($class, $storeid, $scfg) = @_;
>> > +    return undef;
>> > +}
>> > +
>> > +=head2 IMAGE OPERATIONS
>> > +
>>
>> should this describe what IMAGES are in the context of PVE? else as a
>> newcomer the difference between IMAGE here and VOLUME below might not
>> be clear..
> 
> Yeah, that's a good idea. Maximiliano and I were also thinking about
> maybe adding a GLOSSARY section at the bottom of the file where certain
> terms could be explained / defined in more detail in general.
> What do you think?
> 
> Alternatively, we could also have the top-level description define the
> most basic of terms, but I don't want to load the docs here with too
> much information up front.

I think it depends - things that are not only interesting for plugin
devs should probably go into docs proper and just be referenced here. I
don't think the distinction between volume and image is very important
outside of development, so that might be explained here?

the POD documentation here is targeted at us and plugin devs, so that
should be the guideline when deciding what goes where - and what goes in
in the first place - I think a focus on "what do I need to know/be aware
of when writing a plugin" (or adapting the interface) should be the
guiding principle for that.

e.g. for that knowing that this parameter is optional doesn't buy me
much. knowing what it means if it is set does (or that I can ignore it
if my storage can't do X, or should die if set in that case)


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel