all lists on lists.proxmox.com
 help / color / mirror / Atom feed
From: Thomas Lamprecht <t.lamprecht@proxmox.com>
To: "Proxmox VE development discussion" <pve-devel@lists.proxmox.com>,
	"Fabian Grünbichler" <f.gruenbichler@proxmox.com>
Cc: Wolfgang Bumiller <w.bumiller@proxmox.com>
Subject: Re: [pve-devel] [PATCH storage] btrfs: check for btrfs in on_add_hook and activate
Date: Thu, 24 Jun 2021 11:27:13 +0200	[thread overview]
Message-ID: <1522a367-35fe-e7ba-a03b-9881d24c1dd7@proxmox.com> (raw)
In-Reply-To: <1624526230.jg5pbz96o8.astroid@nora.none>

On 24.06.21 11:23, Fabian Grünbichler wrote:
> On June 24, 2021 11:10 am, Thomas Lamprecht wrote:
>> On 24.06.21 09:56, Fabian Grünbichler wrote:
>>> On June 24, 2021 9:29 am, Wolfgang Bumiller wrote:
>>>>  sub activate_storage {
>>>>      my ($class, $storeid, $scfg, $cache) = @_;
>>>> +    assert_btrfs($scfg->{path});
>>>>      return PVE::Storage::DirPlugin::activate_storage($class, $storeid, $scfg, $cache);
>>> shouldn't this be the other way round? first check for things like 
>>> is_mountpoint, then whether btrfs is there.. makes for less confusing 
>>> error message at least..
>>>
>>
>> But then we create already the sub-directories in DirPlugin's SUPER->activate_storage call
>> to the base plugin one and leave that stuff over when the assert fails?
>>
> 
> true. but OTOH, we do support dir storages where $path does not exist 
> yet before the first activation..
> 
> maybe
> 
> if is_mountpoint check that mountpoint // path with DirPlugin::path_is_mounted && btrfs
> 
> then call activate_storage from dir plugin
> 
> then check $path is btrfs
> 
> most setups should have is_mountpoint set (except maybe / on btrfs with 
> no separate "data" filesystem..), so this should handle most of it. if 
> we pull in the mkdir $path handling into the BTRFSPlugin, then 
> everything would be handled (and only the subdir creation is delegate to 
> the DirPlugin..)
> 

I just duplicated the DirPlugin activate storage, could be factored out maybe but
for now I prefer it as is, using actual plugin methods from other plugins feels
always a bit weird and risky, as on the use-site one is seldom aware of that when
changing things there, risking breakage - so in the longer term I'd like that the
more generic stuff moves to a "static" helper module, not having a export-base.




      reply	other threads:[~2021-06-24  9:27 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-06-24  7:29 Wolfgang Bumiller
2021-06-24  7:56 ` Fabian Grünbichler
2021-06-24  8:43   ` Wolfgang Bumiller
2021-06-24  9:10   ` Thomas Lamprecht
2021-06-24  9:23     ` Fabian Grünbichler
2021-06-24  9:27       ` Thomas Lamprecht [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1522a367-35fe-e7ba-a03b-9881d24c1dd7@proxmox.com \
    --to=t.lamprecht@proxmox.com \
    --cc=f.gruenbichler@proxmox.com \
    --cc=pve-devel@lists.proxmox.com \
    --cc=w.bumiller@proxmox.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal