Re: [Qemu-devel] [PATCH v4 2/3] Extract code to nbd_setup function to be used for many purposes

[Chunyan Liu]

2011/12/5 Stefan Hajnoczi <address@hidden>

On Mon, Dec 5, 2011 at 5:46 AM, Chunyan Liu <address@hidden> wrote:
> 2011/12/3 Paolo Bonzini <address@hidden>
>>
>> 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