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

[Stefan Hajnoczi]

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.

The sigterm_fd[] variable could also be handled more cleanly.
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.  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.

Stefan