All of lore.kernel.org
 help / color / mirror / Atom feed
From: Dave Wysochanski <dwysocha@redhat.com>
To: lvm-devel@redhat.com
Subject: [PATCH] Another take on vg_read.
Date: Thu, 22 Jan 2009 12:36:50 -0500	[thread overview]
Message-ID: <1232645811.8897.6.camel@localhost.localdomain> (raw)
In-Reply-To: <1232619010-4858-1-git-send-email-prockai@redhat.com>

I went through the list of earlier comments.  Here's what I found that
seems to not have been addressed:


Agk's earlier comments still unaddressed:
1. PATCH6 (old patch#4/11). vg_read_error().
use a typedef for the return value of vg_read_error(), or explain what the
uint32_t means 
https://www.redhat.com/archives/lvm-devel/2009-January/msg00039.html

2. PATCH2. flag naming/definition.
Use a common prefix so they get grouped together.  Line up the 0x0000 parts
vertically making their "single bit" nature more obvious.
https://www.redhat.com/archives/lvm-devel/2009-January/msg00036.html
 
3. PATCH2. EXISTENCE_CHECK definition.
Ambiguous meaning - which is it?  1. self-evident (delete).  2. reserved for
future (say 'reserved').
https://www.redhat.com/archives/lvm-devel/2009-January/msg00036.html

4. PATCH2. _vg_make_handle
naming and possible 'failure' typedef
https://www.redhat.com/archives/lvm-devel/2009-January/msg00037.html

5. PATCH2. _recover_vg()
[note to check call paths later if more than one VG lock is held simultaneously]
https://www.redhat.com/archives/lvm-devel/2009-January/msg00037.html

6. PATCH2. Flag definition.
Some flags (ALLOW_INCONSISTENT) still need comment definitions.
https://www.redhat.com/archives/lvm-devel/2009-January/msg00037.html

7. PATCH2. read_failed member
Let's add a brief comment explaining this one.
read_failed ? Yes or No.  Rename it perhaps?
I think it should be completely separate from 'status' and 'alloc'.
https://www.redhat.com/archives/lvm-devel/2009-January/msg00037.html

8. PATCH2 (old patch#3/11) _vg_check_status
typedef for the return value?
https://www.redhat.com/archives/lvm-devel/2009-January/msg00038.html


Future cleanup:
1. FAILED_READ_ONLY
Note only FAILED_READ_ONLY is used by any tool today - only vgreduce.
Further, FAILED_READ_ONLY can only occur with LVM1 metadata as far as I
can tell.

2. PATCH2. _vg_lock_and_read failure path with invalid name returns NULL.
Note that callling vg_read_error() after hitting this path will return
FAILED_ALLOCATION.  Do we need FAILED_INVALID_NAME or perhaps re-use
FAILED_NOT_FOUND?
https://www.redhat.com/archives/lvm-devel/2009-January/msg00040.html


On Thu, 2009-01-22 at 11:09 +0100, Petr Rockai wrote:
> Hi,
> 
> I have done some more work on this series. I am resending everything, to avoid
> sequencing problems like I introduced last time with a partial send.
> 
> Anyway, some news:
> - the vg_check_status CLUSTERED fallthrough is fixed to return immediately
> - cluster locking is now properly enforced by vg_read/vg_read_for_update
> - process_each_pv is also ported over to new vg_read
> - vg_read_internal is now private in metadata.c (static _vg_read_internal)
> (plus, all patches should apply cleanly on top of today's CVS)
> 
> Hopefully, this moves things forward a little.
> 
> Yours,
>    Petr.
> 
> --
> lvm-devel mailing list
> lvm-devel at redhat.com
> https://www.redhat.com/mailman/listinfo/lvm-devel



  parent reply	other threads:[~2009-01-22 17:36 UTC|newest]

Thread overview: 26+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2009-01-22 10:09 [PATCH] Another take on vg_read Petr Rockai
2009-01-22 10:09 ` [PATCH 1/14] Move vg_read out of the way, renaming it to vg_read_internal Petr Rockai
2009-01-22 10:09   ` [PATCH 2/14] Provide _vg_lock_and_read, a new implementation of lock+vg_read_internal Petr Rockai
2009-01-22 10:09     ` [PATCH 3/14] Properly enforce cluster locking in _vg_lock_and_read Petr Rockai
2009-01-22 10:10       ` [PATCH 4/14] Replace implementation of vg_check_status with a call to _vg_check_status Petr Rockai
2009-01-22 10:10         ` [PATCH 5/14] Implement vg_read and vg_read_for_update Petr Rockai
2009-01-22 10:10           ` [PATCH 6/14] Add vg_read_error and vg_might_exist Petr Rockai
2009-01-22 10:10             ` [PATCH 7/14] Convert the straight instances of vg_lock_and_read to new vg_read(_for_update) Petr Rockai
2009-01-22 10:10               ` [PATCH 8/14] Convert vgreduce to use vg_read_for_update Petr Rockai
2009-01-22 10:10                 ` [PATCH 9/14] Convert vgrename to vg_read_for_update Petr Rockai
2009-01-22 10:10                   ` [PATCH 10/14] Convert vgsplit to use vg_read_for_update Petr Rockai
2009-01-22 10:10                     ` [PATCH 11/14] Rework the toollib interface (process_each_*) on top of new vg_read Petr Rockai
2009-01-22 10:10                       ` [PATCH 12/14] Port process_each_pv to " Petr Rockai
2009-01-22 10:10                         ` [PATCH 13/14] Remove now-unused vg_lock_and_read Petr Rockai
2009-01-22 10:10                           ` [PATCH 14/14] Un-export vg_read_internal Petr Rockai
2009-01-26 16:05                             ` Dave Wysochanski
2009-01-22 16:13                         ` [PATCH 12/14] Port process_each_pv to new vg_read Dave Wysochanski
2009-01-24 21:45                 ` [PATCH updated] Convert vgreduce to use vg_read_for_update Dave Wysochanski
2009-01-24 21:46                   ` [PATCH] " Dave Wysochanski
2009-01-25  0:21                     ` [PATCH update2] " Dave Wysochanski
2009-01-25  0:21                       ` [PATCH] " Dave Wysochanski
2009-01-22 18:17       ` [PATCH 3/14] Properly enforce cluster locking in _vg_lock_and_read Dave Wysochanski
2009-01-22 20:15         ` Petr Rockai
2009-02-06 18:21     ` [PATCH 2/14] Provide _vg_lock_and_read, a new implementation of lock+vg_read_internal Dave Wysochanski
2009-01-22 17:36 ` Dave Wysochanski [this message]
2009-01-22 17:48   ` [PATCH] Another take on vg_read Petr Rockai

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=1232645811.8897.6.camel@localhost.localdomain \
    --to=dwysocha@redhat.com \
    --cc=lvm-devel@redhat.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.