2011/12/5 Stefan Hajnoczi <stefa...@gmail.com>
> On Mon, Dec 5, 2011 at 5:46 AM, Chunyan Liu <cy...@suse.com> wrote:
> > 2011/12/3 Paolo Bonzini <pbonz...@redhat.com>
> >>
> >> On 12/02/2011 04:27 PM, Chunyan Liu wrote:
> >>>
> >>> @@ -42,6 +42,18 @@ static int verbose;
> >>> static char *device;
> >>> static char *srcpath;
> >>> static char *sockpath;
> >>> +static int is_sockpath_option;
> >>> +static int sigterm_fd[2];
> >>> +static off_t dev_offset;
> >>> +static uint32_t nbdflags;
> >>> +static bool disconnect;
> >>> +static const char *bindto = "0.0.0.0";
> >>> +static int port = NBD_DEFAULT_PORT;
> >>> +static int li;
> >>> +static int flags = BDRV_O_RDWR;
> >>> +static int partition = -1;
> >>> +static int shared = 1;
> >>> +static int persistent;
> >>
> >>
> >> A lot of statics... "li" seems unused.
> >
> >
> > Using these statics simply because most of them are global parameters
> > getting from command line options, will be used later. Otherwise, the
> > nbd_setup() function should take many parameters.
> >
> > Ahh, "li" could be defined in main(). After getting parameters from
> option,
> > later places can use "port".
> > case 'p':
> > li = strtol(optarg, &end, 0);
> > if (*end) {
> > errx(EXIT_FAILURE, "Invalid port `%s'", optarg);
> > }
> > if (li < 1 || li > 65535) {
> > errx(EXIT_FAILURE, "Port out of range `%s'", optarg);
> > }
> > port = (uint16_t)li;
>
> You can also move the bdrv_new()/bdrv_open()/bdrv_delete() out of the
> nbd_setup() function.
>
Sure. But different parameters needed by nbd_setup(). (bs, dev_offset,
fd_size instead of srcpath, dev_offset, partition, flags).
The sigterm_fd[] variable could also be handled more cleanly.
>
Any suggestion?
> Basically, I'd rather see a bigger patch that arranges things nicely
> than chopping the function in half and making most variables global in
> order to have a shorter patch.
Currently, the nbd_setup needs parameters: device, srcpath, flags,
partition, dev_offset, nbdflags, sockpath, bindto, port, shared,
persistent, verbose, sigterm_rfd. More than 10 parameters. I still didn't
find a better way to reduce parameters. Making variables global is a
workaround to avoid nbd_setup taking too many parameters. Actually, except
for sigterm_rfd, all others are pared from command line options.
To improve, what I can think of is to define a structure to save those
values parsed from command line options and pass that structure to
nbd_setup as a parameter. Of course, bdrv_new/open/delete can be moved out,
just passing more parameters to nbd_setup. Temporarily, I couldn't think of
others. Do you have any better ideas?
We need to maintain this code so if it
> needs to be rearranged in order to fit with the new requirements then
> that's worth doing so it will be easier to read and maintain in the
> future.
nbd_setup() is a slightly confusing name because the entire main loop
> for the executes in the function. Perhaps nbd_run() is better.
No problem, could be changed.
> Stefan
>
>
