On Wed Mar 20, 2024 at 9:05 AM CET, Fabian Grünbichler wrote:
> > Max Carrara <m.carr...@proxmox.com> hat am 19.03.2024 18:41 CET geschrieben:
> > On Tue Mar 19, 2024 at 11:04 AM CET, Fabian Grünbichler wrote:
> > > On March 5, 2024 4:07 pm, Max Carrara wrote:
> > > > diff --git a/bin/pve-init-ceph-crash b/bin/pve-init-ceph-crash
> > > > new file mode 100755
> > > > index 00000000..c8e9217d
> > > > --- /dev/null
> > > > +++ b/bin/pve-init-ceph-crash
> > > > @@ -0,0 +1,129 @@
> > > > +#!/usr/bin/perl
> > > > +
> > > > +use strict;
> > > > +use warnings;
> > > > +
> > > > +use List::Util qw(first);
> > > > +
> > > > +use PVE::Ceph::Tools;
> > > > +use PVE::Cluster;
> > > > +use PVE::RADOS;
> > > > +use PVE::RPCEnvironment;
> > > > +
> > > > +my $ceph_cfg_file = 'ceph.conf';
> > > > +my $keyring_value = '/etc/pve/ceph/$cluster.$name.keyring';
> > > > +
> > > > +my $entity = 'client.crash';
> > >
> > > nit: this could be inlined?
> >
> > Inlined as in use 'client.crash' as the key directly? Eh, I'm more a fan
> > of using a variable here, as constantly spelling it out while changing a
> > bunch of things gets a little painful ...
> >
> > If you meant that I should put it in `try_adapt_cfg` - sure, I missed
> > that that's the only `sub` in which it's being used, woops!
>
> both would be fine for me :)
ACK!
>
> > > > +
> > > > +
>
> [..]
>
> > > > +
> > > > +sub main {
> > > > + my $rpcenv = PVE::RPCEnvironment->init('cli');
> > > > +
> > > > + $rpcenv->init_request();
> > > > + $rpcenv->set_language($ENV{LANG});
> > > > + $rpcenv->set_user('root@pam');
> > >
> > > why do we need an rpcenv here?
> >
> > Double-checked, just to be sure: `librados-perl` requires an
> > `RPCEnvironment` to do some handling regarding forks -
> > `RPCEnvironment::get()` will die if no env was initialized.
>
> good to know. maybe a comment makes sense - the rpcenv is not used by
> anything else here it seems, and the fact that librados uses it because it
> forks itself and wants to clean up in case it is called in an API server
> process is not obvious ;)
>
> you could replace those lines with a call to setup_default_cli_env that does
> all of the above and the "am I root" check in one ;)
Also ACK! Thanks again for your input!
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
