From: Carlos Maiolino <cmaiolino@redhat.com>
To: cluster-devel.redhat.com
Subject: [Cluster-devel] [PATCH] mkfs: Remove duplicated code from verify_bsize()
Date: Mon, 18 Jul 2011 15:44:33 -0300 [thread overview]
Message-ID: <20110718184433.GA3160@andromeda.usersys.redhat.com> (raw)
In-Reply-To: <4E1C5C46.5050205@redhat.com>
My apologies for my delay, I was on PTO.
> are_you_sure() doesn't really belong in libgfs2 because it's user
> interface code and it has exit() calls (via die()). You might want
> to read bz408631 to see where libgfs2 is heading.
>
Gotcha will look.
> >+/*Function prototypes*/
>
> Minor point, but I personally dislike comments like this. Everyone
> knows what a function prototype looks like :)
>
Indeed. I added it mainly to "make it clear" but I agree in do not add
this comment.
> I'm not sure that it is worth using are_you_sure() here, as it opens
> the device again and calls check_dev_content() (which calls pipe()
> and then fork() ...), which is a lot more overhead than the few
> lines that you're removing here. Perhaps you could change
> are_you_sure to be more generic and move the checking into a
> separate function?
>
Looks like both (Steve and Andy) agrees to remove open() call from
are_you_sure() function, so, looks that's the best way to fix this up
and maybe as Andy suggestion set up another function to test device open
if necessary.
My concern here btw, is if there is any problem in add the function prototype
to the beginning of the file.
What you guys think?
--
--Carlos
next prev parent reply other threads:[~2011-07-18 18:44 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2011-07-12 14:06 [Cluster-devel] [PATCH] mkfs: Remove duplicated code from verify_bsize() Carlos Maiolino
2011-07-12 14:26 ` Steven Whitehouse
2011-07-12 14:37 ` Andrew Price
2011-07-18 18:44 ` Carlos Maiolino [this message]
2011-07-19 9:40 ` Steven Whitehouse
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=20110718184433.GA3160@andromeda.usersys.redhat.com \
--to=cmaiolino@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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).