On 03/21/14 at 04:02pm, Baoquan He wrote:
On 03/20/14 at 10:14am, Vivek Goyal wrote:
> On Thu, Mar 20, 2014 at 01:57:12PM +0800, Baoquan He wrote:
> Thanks for the patches bao. They are almost ready. Some minor
nits.
>
> In general, let us put some comments before helper functions. Which
> tells user what does a function do, what arguments does it expect and
> what's the expected outcome.
>
> You don't have to do it for really simple functions but if something
> is little more twisted, we need to do it so that code reuse later is
> easier.
>
> >
> > +make_save_path_fs()
>
> For example, for this function we need to explain what is "target". Looks
> like it could be UUID, LABEL, block device or even nfs server export of
> the form of "my.server.com:/tmp/export"?
>
> Given the fact that we are creating an absolute path from dump target
> and path variable, may be a better name could be.
>
> get_absolute_save_path().
Yes, will change.
>
> And possibly this could be used for both default case as well as when
> dump taret is specified.
>
> When dump target is not specified, then $target would be null.
Well, after rethink I got your meaning. Even though default target with
no mount, this function can also be called. This can make code be
consistent on style.
You mean in this function check target is specified explicitly or is a
default target. If you mean this, it doesn't have to pass target as
argument. Otherwise the target always need be passed in. Because it's
done outside of this fucntion to check whether target is a specified
target or a default target.
>
> Rest of the patch looks good to me.
>
> Thanks
> Vivek
>