Thanks, Paolo!
2012/1/18 Paolo Bonzini <pbonz...@redhat.com>:
> On 01/18/2012 09:48 AM, Chunyan Liu wrote:
>>
>> Stefan, could you help commit it if it's OK? Thanks.
>> Same as in thread:
>> http://lists.gnu.org/archive/html/qemu-devel/2011-12/msg01083.html
>> but rebase it to latest code.
>>
>> Add -f option to qemu-nbd to find a free nbd device for user and connect
>> disk image to that device.
>
>
> Hi Chunyan,
>
> I'm now the NBD maintainer so I can take care of the patch. There are still
> a couple of nits.
>
> The main problem is that the device name is never printed, which requires
> some imagination on part of the user. :)
>
> This pretty much requires that you set "verbose = 1" together with -f. This
> disables daemonization, unfortunately, but there isn't really any better way
> to do so. I'm inclined towards removing daemonization altogether from 1.1;
> other opinions are welcome.
Currently, to print the connected device name, we can also change code a little
as following: ("-v" can still indicate working on background or not. )
@@ -256,10 +256,9 @@ static void *nbd_client_thread(void *arg)
/* update partition table */
pthread_create(&show_parts_thread, NULL, show_parts, NULL);
- if (verbose) {
- fprintf(stderr, "NBD device %s is now connected to %s\n",
+ fprintf(stderr, "NBD device %s is now connected to %s\n",
device, srcpath);
- } else {
+ if (!verbose) {
/* Close stderr so that the qemu-nbd process exits. */
dup2(STDOUT_FILENO, STDERR_FILENO);
}
>
> Anyhow, please set "verbose = 1" and change the help text for -c/-v from
>
> " -c, --connect=DEV connect FILE to the local NBD device DEV\n"
>
> " -v, --verbose display extra debugging information\n"
>
> to
>
> " -c, --connect=DEV connect FILE to the local NBD device DEV\n"
> " and run in the background\n"
>
> " -v, --verbose with -c, display extra information and\n"
> " do not run in the background\n"
>
> Since a respin is required, there are a couple of other comments inline.
>
>> syntax: qemu-nbd -f disk.img
>> ---
>> qemu-nbd.c | 76
>> ++++++++++++++++++++++++++++++++++++++++++-----------------
>> 1 files changed, 54 insertions(+), 22 deletions(-)
>>
>> diff --git a/qemu-nbd.c b/qemu-nbd.c
>> index eb61c33..f4c1437 100644
>> --- a/qemu-nbd.c
>> +++ b/qemu-nbd.c
>> @@ -33,7 +33,7 @@
>> #include<libgen.h>
>> #include<pthread.h>
>>
>> -#define SOCKET_PATH "/var/lock/qemu-nbd-%s"
>> +#define SOCKET_PATH "/var/lock/qemu-nbd-%d"
>>
>> static NBDExport *exp;
>> static int verbose;
>> @@ -55,12 +55,13 @@ static void usage(const char *name)
>> " -o, --offset=OFFSET offset into the image\n"
>> " -b, --bind=IFACE interface to bind to (default `0.0.0.0')\n"
>> " -k, --socket=PATH path to the unix socket\n"
>> -" (default '"SOCKET_PATH"')\n"
>> +" (default /var/lock/qemu-nbd-PID)\n"
>> " -r, --read-only export read-only\n"
>> " -P, --partition=NUM only expose partition NUM\n"
>> " -s, --snapshot use snapshot file\n"
>> " -n, --nocache disable host cache\n"
>> " -c, --connect=DEV connect FILE to the local NBD device DEV\n"
>> +" -f, --find find a free NBD device and connect FILE to it\n"
>> " -d, --disconnect disconnect the specified device\n"
>> " -e, --shared=NUM device can be shared by NUM clients (default
>> '1')\n"
>> " -t, --persistent don't exit on the last connection\n"
>> @@ -69,7 +70,7 @@ static void usage(const char *name)
>> " -V, --version output version information and exit\n"
>> "\n"
>> "Report bugs to<anth...@codemonkey.ws>\n"
>> - , name, NBD_DEFAULT_PORT, "DEVICE");
>> + , name, NBD_DEFAULT_PORT);
>> }
>>
>> static void version(const char *name)
>> @@ -194,7 +195,8 @@ static void *show_parts(void *arg)
>>
>> static void *nbd_client_thread(void *arg)
>> {
>> - int fd = *(int *)arg;
>> + int fd = -1;
>> + int find = *(int *)arg;
>> off_t size;
>> size_t blocksize;
>> uint32_t nbdflags;
>> @@ -213,9 +215,42 @@ static void *nbd_client_thread(void *arg)
>> goto out;
>> }
>>
>> - ret = nbd_init(fd, sock, nbdflags, size, blocksize);
>> - if (ret == -1) {
>> - goto out;
>> + if (!find) {
>> + fd = open(device, O_RDWR);
>> + if (fd == -1) {
>> + fprintf(stderr, "Failed to open %s\n", device);
>> + goto out;
>> + }
>> +
>> + ret = nbd_init(fd, sock, nbdflags, size, blocksize);
>> + if (ret == -1) {
>> + goto out;
>> + }
>> + } else {
>> + int i = 0;
>> + int max_nbd = 16;
>> + device = g_malloc(64);
>> +
>> + for (i = 0; i< max_nbd; i++) {
>> + snprintf(device, 64, "/dev/nbd%d", i);
>> + fd = open(device, O_RDWR);
>> + if (fd == -1) {
>> + continue;
>> + }
>> +
>> + if (nbd_init(fd, sock, nbdflags, size, blocksize) == -1) {
>> + close(fd);
>> + continue;
>> + }
>> +
>> + break;
>> + }
>> +
>> + if (i>= max_nbd) {
>> + fprintf(stderr, "Fail to find a free nbd device\n");
>> + g_free(device);
>> + goto out;
>
>
> Just use errx(EXIT_FAILURE, "Could not find a free NBD device.") here.
Using errx() will skip kill(getpid(), SIGTERM) and exit directly;
I'm not sure if that will cause problem?
>
>> + }
>> }
>>
>> /* update partition table */
>> @@ -275,7 +310,7 @@ int main(int argc, char **argv)
>> const char *bindto = "0.0.0.0";
>> int port = NBD_DEFAULT_PORT;
>> off_t fd_size;
>> - const char *sopt = "hVb:o:p:rsnP:c:dvk:e:t";
>> + const char *sopt = "hVb:o:p:rsnP:c:dvk:e:tf";
>> struct option lopt[] = {
>> { "help", 0, NULL, 'h' },
>> { "version", 0, NULL, 'V' },
>> @@ -292,6 +327,7 @@ int main(int argc, char **argv)
>> { "shared", 1, NULL, 'e' },
>> { "persistent", 0, NULL, 't' },
>> { "verbose", 0, NULL, 'v' },
>> + { "find", 0, NULL, 'f'},
>> { NULL, 0, NULL, 0 }
>> };
>> int ch;
>> @@ -303,6 +339,7 @@ int main(int argc, char **argv)
>> int ret;
>> int fd;
>> int persistent = 0;
>> + int find = 0;
>> pthread_t client_thread;
>>
>> /* The client thread uses SIGTERM to interrupt the server. A signal
>> @@ -380,6 +417,9 @@ int main(int argc, char **argv)
>> case 'v':
>> verbose = 1;
>> break;
>> + case 'f':
>> + find = 1;
>> + break;
>> case 'V':
>> version(argv[0]);
>> exit(0);
>> @@ -414,7 +454,7 @@ int main(int argc, char **argv)
>> return 0;
>> }
>
>
> Please give an error here if "device && find" ("The -f option is
> incompatible with -c").
>
>> - if (device&& !verbose) {
>> + if ((device || find)&& !verbose) {
>> int stderr_fd[2];
>> pid_t pid;
>> int ret;
>> @@ -466,18 +506,10 @@ int main(int argc, char **argv)
>> }
>> }
>>
>> - if (device) {
>> - /* Open before spawning new threads. In the future, we may
>> - * drop privileges after opening.
>> - */
>> - fd = open(device, O_RDWR);
>> - if (fd == -1) {
>> - err(EXIT_FAILURE, "Failed to open %s", device);
>> - }
>> -
>> + if (device || find) {
>> if (sockpath == NULL) {
>> sockpath = g_malloc(128);
>> - snprintf(sockpath, 128, SOCKET_PATH, basename(device));
>> + snprintf(sockpath, 128, SOCKET_PATH, getpid());
>> }
>> }
>>
>> @@ -510,10 +542,10 @@ int main(int argc, char **argv)
>> return 1;
>> }
>>
>> - if (device) {
>> + if (device || find) {
>> int ret;
>>
>> - ret = pthread_create(&client_thread, NULL,
>> nbd_client_thread,&fd);
>> + ret = pthread_create(&client_thread, NULL,
>> nbd_client_thread,&find);
>
>
> Just pass NULL here and test "device != NULL" in the nbd_client_thread.
>
>> if (ret != 0) {
>> errx(EXIT_FAILURE, "Failed to create client thread: %s",
>> strerror(ret));
>> @@ -536,7 +568,7 @@ int main(int argc, char **argv)
>> unlink(sockpath);
>> }
>>
>> - if (device) {
>> + if (device || find) {
>> void *ret;
>> pthread_join(client_thread,&ret);
>> exit(ret != NULL);
>
>
> Thanks,
>
> Paolo
>
