subject:"Re\: \[Qemu\-devel\] \[PATCH\] block\: add native support for NFS"

Re: [Qemu-devel] [PATCH] block: add native support for NFS

2013-12-17 Thread Fam Zheng


On 2013年12月17日 15:53, Peter Lieven wrote:

Hi Fam,

On 17.12.2013 05:07, Fam Zheng wrote:

On 2013年12月16日 23:34, Peter Lieven wrote:

+static int coroutine_fn nfs_co_writev(BlockDriverState *bs,
+int64_t sector_num, int
nb_sectors,
+QEMUIOVector *iov)
+{
+nfsclient *client = bs-opaque;
+struct NFSTask Task;
+char *buf = NULL;
+
+nfs_co_init_task(client, Task);
+
+buf = g_malloc(nb_sectors * BDRV_SECTOR_SIZE);
+qemu_iovec_to_buf(iov, 0, buf, nb_sectors * BDRV_SECTOR_SIZE);
+
+if (nfs_pwrite_async(client-context, client-fh,
+ sector_num * BDRV_SECTOR_SIZE,
+ nb_sectors * BDRV_SECTOR_SIZE,
+ buf, nfs_co_generic_cb, Task) != 0) {
+g_free(buf);
+return -EIO;
+}
+
+while (!Task.complete) {
+nfs_set_events(client);
+qemu_coroutine_yield();
+}
+
+g_free(buf);
+
+if (Task.status != nb_sectors * BDRV_SECTOR_SIZE) {
+return -EIO;
+}
+
+bs-total_sectors = MAX(bs-total_sectors, sector_num +
nb_sectors);
+client-allocated_file_size = -ENOTSUP;


Why does allocated_file_size become not supported after a write?

I thought that someone would ask this ;-) bdrv_allocated_file_size is only
used in image info. I saved some code here implementing an async call.
On open I fstat anyway and store that value. For qemu-img info this is
sufficient, but the allocated size likely changes after a write. -ENOTSUP
is the default if bdrv_allocated_file_size is not implemented.


OK. Please add some comment here.


+static int nfs_file_open_common(BlockDriverState *bs, QDict
*options, int flags,
+ int open_flags, Error **errp)
+{
+nfsclient *client = bs-opaque;
+const char *filename;
+int ret = 0;
+QemuOpts *opts;
+Error *local_err = NULL;
+char *server = NULL, *path = NULL, *file = NULL, *strp;
+struct stat st;
+
+opts = qemu_opts_create_nofail(runtime_opts);
+qemu_opts_absorb_qdict(opts, options, local_err);
+if (error_is_set(local_err)) {
+qerror_report_err(local_err);
+error_free(local_err);
+ret = -EINVAL;
+goto fail;
+}
+
+filename = qemu_opt_get(opts, filename);
+
+client-context = nfs_init_context();
+
+if (client-context == NULL) {
+error_setg(errp, Failed to init NFS context);
+ret = -EINVAL;
+goto fail;
+}
+
+server = g_strdup(filename + 6);


Please check the length of filename is longer than 6 before accessing
filename[6].

Good point. I will check for this, but in fact I think it can't happen
because we will
never end up there if filename does not start with nfs://


True, at least for now, but it doesn't hurt to be defensive, especially 
with strings.





+if (server[0] == '/' || server[0] == '\0') {
+error_setg(errp, Invalid server in URL);
+ret = -EINVAL;
+goto fail;
+}
+strp = strchr(server, '/');
+if (strp == NULL) {
+error_setg(errp, Invalid URL specified.\n);
+ret = -EINVAL;
+goto fail;
+}
+path = g_strdup(strp);
+*strp = 0;
+strp = strrchr(path, '/');
+if (strp == NULL) {
+error_setg(errp, Invalid URL specified.\n);
+ret = -EINVAL;
+goto fail;
+}
+file = g_strdup(strp);
+*strp = 0;
+
+if (nfs_mount(client-context, server, path) != 0) {
+error_setg(errp, Failed to mount nfs share: %s,
+nfs_get_error(client-context));
+ret = -EINVAL;
+goto fail;
+}
+
+if (open_flags  O_CREAT) {
+if (nfs_creat(client-context, file, 0600, client-fh) != 0) {
+error_setg(errp, Failed to create file: %s,
+ nfs_get_error(client-context));
+ret = -EINVAL;
+goto fail;
+}
+} else {
+open_flags = (flags  BDRV_O_RDWR) ? O_RDWR : O_RDONLY;
+if (nfs_open(client-context, file, open_flags, client-fh)
!= 0) {
+error_setg(errp, Failed to open file : %s,
+   nfs_get_error(client-context));
+ret = -EINVAL;
+goto fail;
+}
+}
+
+if (nfs_fstat(client-context, client-fh, st) != 0) {
+error_setg(errp, Failed to fstat file: %s,
+   nfs_get_error(client-context));
+ret = -EIO;
+goto fail;
+}
+
+bs-total_sectors = st.st_size / BDRV_SECTOR_SIZE;


Please use DIV_ROUND_UP(). Otherwise the remainder in last sector
couldn't be read.

Will do. Can't it happen that we end up reading unallocated sectors?


Hmm, maybe. Not missing last bytes of unaligned sector is essential for 
VMDK description file. But you are right, please add check code and make 
sure that we don't read beyond EOF as well.



Thanks for reviewing!

One wish as I think you are the VMDK format maintainer. Can you rework
vmdk_create and possible
other

Re: [Qemu-devel] [PATCH] block: add native support for NFS

2013-12-17 Thread Peter Lieven


On 17.12.2013 09:29, Fam Zheng wrote:

On 2013年12月17日 15:53, Peter Lieven wrote:

Hi Fam,

On 17.12.2013 05:07, Fam Zheng wrote:

On 2013年12月16日 23:34, Peter Lieven wrote:

+static int coroutine_fn nfs_co_writev(BlockDriverState *bs,
+int64_t sector_num, int
nb_sectors,
+QEMUIOVector *iov)
+{
+nfsclient *client = bs-opaque;
+struct NFSTask Task;
+char *buf = NULL;
+
+nfs_co_init_task(client, Task);
+
+buf = g_malloc(nb_sectors * BDRV_SECTOR_SIZE);
+qemu_iovec_to_buf(iov, 0, buf, nb_sectors * BDRV_SECTOR_SIZE);
+
+if (nfs_pwrite_async(client-context, client-fh,
+ sector_num * BDRV_SECTOR_SIZE,
+ nb_sectors * BDRV_SECTOR_SIZE,
+ buf, nfs_co_generic_cb, Task) != 0) {
+g_free(buf);
+return -EIO;
+}
+
+while (!Task.complete) {
+nfs_set_events(client);
+qemu_coroutine_yield();
+}
+
+g_free(buf);
+
+if (Task.status != nb_sectors * BDRV_SECTOR_SIZE) {
+return -EIO;
+}
+
+bs-total_sectors = MAX(bs-total_sectors, sector_num +
nb_sectors);
+client-allocated_file_size = -ENOTSUP;


Why does allocated_file_size become not supported after a write?

I thought that someone would ask this ;-) bdrv_allocated_file_size is only
used in image info. I saved some code here implementing an async call.
On open I fstat anyway and store that value. For qemu-img info this is
sufficient, but the allocated size likely changes after a write. -ENOTSUP
is the default if bdrv_allocated_file_size is not implemented.


OK. Please add some comment here.


+static int nfs_file_open_common(BlockDriverState *bs, QDict
*options, int flags,
+ int open_flags, Error **errp)
+{
+nfsclient *client = bs-opaque;
+const char *filename;
+int ret = 0;
+QemuOpts *opts;
+Error *local_err = NULL;
+char *server = NULL, *path = NULL, *file = NULL, *strp;
+struct stat st;
+
+opts = qemu_opts_create_nofail(runtime_opts);
+qemu_opts_absorb_qdict(opts, options, local_err);
+if (error_is_set(local_err)) {
+qerror_report_err(local_err);
+error_free(local_err);
+ret = -EINVAL;
+goto fail;
+}
+
+filename = qemu_opt_get(opts, filename);
+
+client-context = nfs_init_context();
+
+if (client-context == NULL) {
+error_setg(errp, Failed to init NFS context);
+ret = -EINVAL;
+goto fail;
+}
+
+server = g_strdup(filename + 6);


Please check the length of filename is longer than 6 before accessing
filename[6].

Good point. I will check for this, but in fact I think it can't happen
because we will
never end up there if filename does not start with nfs://


True, at least for now, but it doesn't hurt to be defensive, especially with 
strings.




+if (server[0] == '/' || server[0] == '\0') {
+error_setg(errp, Invalid server in URL);
+ret = -EINVAL;
+goto fail;
+}
+strp = strchr(server, '/');
+if (strp == NULL) {
+error_setg(errp, Invalid URL specified.\n);
+ret = -EINVAL;
+goto fail;
+}
+path = g_strdup(strp);
+*strp = 0;
+strp = strrchr(path, '/');
+if (strp == NULL) {
+error_setg(errp, Invalid URL specified.\n);
+ret = -EINVAL;
+goto fail;
+}
+file = g_strdup(strp);
+*strp = 0;
+
+if (nfs_mount(client-context, server, path) != 0) {
+error_setg(errp, Failed to mount nfs share: %s,
+nfs_get_error(client-context));
+ret = -EINVAL;
+goto fail;
+}
+
+if (open_flags  O_CREAT) {
+if (nfs_creat(client-context, file, 0600, client-fh) != 0) {
+error_setg(errp, Failed to create file: %s,
+ nfs_get_error(client-context));
+ret = -EINVAL;
+goto fail;
+}
+} else {
+open_flags = (flags  BDRV_O_RDWR) ? O_RDWR : O_RDONLY;
+if (nfs_open(client-context, file, open_flags, client-fh)
!= 0) {
+error_setg(errp, Failed to open file : %s,
+   nfs_get_error(client-context));
+ret = -EINVAL;
+goto fail;
+}
+}
+
+if (nfs_fstat(client-context, client-fh, st) != 0) {
+error_setg(errp, Failed to fstat file: %s,
+   nfs_get_error(client-context));
+ret = -EIO;
+goto fail;
+}
+
+bs-total_sectors = st.st_size / BDRV_SECTOR_SIZE;


Please use DIV_ROUND_UP(). Otherwise the remainder in last sector
couldn't be read.

Will do. Can't it happen that we end up reading unallocated sectors?


Hmm, maybe. Not missing last bytes of unaligned sector is essential for VMDK 
description file. But you are right, please add check code and make sure that 
we don't read beyond EOF as well.

Actually it would like to keep bs-total_sectors = st.st_size / 
BDRV_SECTOR_SIZE; for

Re: [Qemu-devel] [PATCH] block: add native support for NFS

2013-12-17 Thread Fam Zheng


于2013年12月17日 星期二 16时46分50秒,Peter Lieven写到:

On 17.12.2013 09:29, Fam Zheng wrote:

On 2013年12月17日 15:53, Peter Lieven wrote:

Hi Fam,

On 17.12.2013 05:07, Fam Zheng wrote:

On 2013年12月16日 23:34, Peter Lieven wrote:

+static int coroutine_fn nfs_co_writev(BlockDriverState *bs,
+int64_t sector_num, int
nb_sectors,
+QEMUIOVector *iov)
+{
+nfsclient *client = bs-opaque;
+struct NFSTask Task;
+char *buf = NULL;
+
+nfs_co_init_task(client, Task);
+
+buf = g_malloc(nb_sectors * BDRV_SECTOR_SIZE);
+qemu_iovec_to_buf(iov, 0, buf, nb_sectors * BDRV_SECTOR_SIZE);
+
+if (nfs_pwrite_async(client-context, client-fh,
+ sector_num * BDRV_SECTOR_SIZE,
+ nb_sectors * BDRV_SECTOR_SIZE,
+ buf, nfs_co_generic_cb, Task) != 0) {
+g_free(buf);
+return -EIO;
+}
+
+while (!Task.complete) {
+nfs_set_events(client);
+qemu_coroutine_yield();
+}
+
+g_free(buf);
+
+if (Task.status != nb_sectors * BDRV_SECTOR_SIZE) {
+return -EIO;
+}
+
+bs-total_sectors = MAX(bs-total_sectors, sector_num +
nb_sectors);
+client-allocated_file_size = -ENOTSUP;


Why does allocated_file_size become not supported after a write?

I thought that someone would ask this ;-) bdrv_allocated_file_size
is only
used in image info. I saved some code here implementing an async call.
On open I fstat anyway and store that value. For qemu-img info this is
sufficient, but the allocated size likely changes after a write.
-ENOTSUP
is the default if bdrv_allocated_file_size is not implemented.


OK. Please add some comment here.


+static int nfs_file_open_common(BlockDriverState *bs, QDict
*options, int flags,
+ int open_flags, Error **errp)
+{
+nfsclient *client = bs-opaque;
+const char *filename;
+int ret = 0;
+QemuOpts *opts;
+Error *local_err = NULL;
+char *server = NULL, *path = NULL, *file = NULL, *strp;
+struct stat st;
+
+opts = qemu_opts_create_nofail(runtime_opts);
+qemu_opts_absorb_qdict(opts, options, local_err);
+if (error_is_set(local_err)) {
+qerror_report_err(local_err);
+error_free(local_err);
+ret = -EINVAL;
+goto fail;
+}
+
+filename = qemu_opt_get(opts, filename);
+
+client-context = nfs_init_context();
+
+if (client-context == NULL) {
+error_setg(errp, Failed to init NFS context);
+ret = -EINVAL;
+goto fail;
+}
+
+server = g_strdup(filename + 6);


Please check the length of filename is longer than 6 before accessing
filename[6].

Good point. I will check for this, but in fact I think it can't happen
because we will
never end up there if filename does not start with nfs://


True, at least for now, but it doesn't hurt to be defensive,
especially with strings.




+if (server[0] == '/' || server[0] == '\0') {
+error_setg(errp, Invalid server in URL);
+ret = -EINVAL;
+goto fail;
+}
+strp = strchr(server, '/');
+if (strp == NULL) {
+error_setg(errp, Invalid URL specified.\n);
+ret = -EINVAL;
+goto fail;
+}
+path = g_strdup(strp);
+*strp = 0;
+strp = strrchr(path, '/');
+if (strp == NULL) {
+error_setg(errp, Invalid URL specified.\n);
+ret = -EINVAL;
+goto fail;
+}
+file = g_strdup(strp);
+*strp = 0;
+
+if (nfs_mount(client-context, server, path) != 0) {
+error_setg(errp, Failed to mount nfs share: %s,
+nfs_get_error(client-context));
+ret = -EINVAL;
+goto fail;
+}
+
+if (open_flags  O_CREAT) {
+if (nfs_creat(client-context, file, 0600, client-fh)
!= 0) {
+error_setg(errp, Failed to create file: %s,
+ nfs_get_error(client-context));
+ret = -EINVAL;
+goto fail;
+}
+} else {
+open_flags = (flags  BDRV_O_RDWR) ? O_RDWR : O_RDONLY;
+if (nfs_open(client-context, file, open_flags, client-fh)
!= 0) {
+error_setg(errp, Failed to open file : %s,
+   nfs_get_error(client-context));
+ret = -EINVAL;
+goto fail;
+}
+}
+
+if (nfs_fstat(client-context, client-fh, st) != 0) {
+error_setg(errp, Failed to fstat file: %s,
+   nfs_get_error(client-context));
+ret = -EIO;
+goto fail;
+}
+
+bs-total_sectors = st.st_size / BDRV_SECTOR_SIZE;


Please use DIV_ROUND_UP(). Otherwise the remainder in last sector
couldn't be read.

Will do. Can't it happen that we end up reading unallocated sectors?


Hmm, maybe. Not missing last bytes of unaligned sector is essential
for VMDK description file. But you are right, please add check code
and make sure that we don't read beyond EOF as well.

Actually it would like to keep bs-total_sectors

Re: [Qemu-devel] [PATCH] block: add native support for NFS

2013-12-17 Thread Peter Lieven


On 17.12.2013 09:51, Fam Zheng wrote:

于2013年 12月17日 星期二 16时46分50秒,Peter Lieven写到:

On 17.12.2013 09:29, Fam Zheng wrote:

On 2013年12月17日 15:53, Peter Lieven wrote:

Hi Fam,

On 17.12.2013 05:07, Fam Zheng wrote:

On 2013年12月16日 23:34, Peter Lieven wrote:

+static int coroutine_fn nfs_co_writev(BlockDriverState *bs,
+int64_t sector_num, int
nb_sectors,
+QEMUIOVector *iov)
+{
+nfsclient *client = bs-opaque;
+struct NFSTask Task;
+char *buf = NULL;
+
+nfs_co_init_task(client, Task);
+
+buf = g_malloc(nb_sectors * BDRV_SECTOR_SIZE);
+qemu_iovec_to_buf(iov, 0, buf, nb_sectors * BDRV_SECTOR_SIZE);
+
+if (nfs_pwrite_async(client-context, client-fh,
+ sector_num * BDRV_SECTOR_SIZE,
+ nb_sectors * BDRV_SECTOR_SIZE,
+ buf, nfs_co_generic_cb, Task) != 0) {
+g_free(buf);
+return -EIO;
+}
+
+while (!Task.complete) {
+nfs_set_events(client);
+qemu_coroutine_yield();
+}
+
+g_free(buf);
+
+if (Task.status != nb_sectors * BDRV_SECTOR_SIZE) {
+return -EIO;
+}
+
+bs-total_sectors = MAX(bs-total_sectors, sector_num +
nb_sectors);
+client-allocated_file_size = -ENOTSUP;


Why does allocated_file_size become not supported after a write?

I thought that someone would ask this ;-) bdrv_allocated_file_size
is only
used in image info. I saved some code here implementing an async call.
On open I fstat anyway and store that value. For qemu-img info this is
sufficient, but the allocated size likely changes after a write.
-ENOTSUP
is the default if bdrv_allocated_file_size is not implemented.


OK. Please add some comment here.


+static int nfs_file_open_common(BlockDriverState *bs, QDict
*options, int flags,
+ int open_flags, Error **errp)
+{
+nfsclient *client = bs-opaque;
+const char *filename;
+int ret = 0;
+QemuOpts *opts;
+Error *local_err = NULL;
+char *server = NULL, *path = NULL, *file = NULL, *strp;
+struct stat st;
+
+opts = qemu_opts_create_nofail(runtime_opts);
+qemu_opts_absorb_qdict(opts, options, local_err);
+if (error_is_set(local_err)) {
+qerror_report_err(local_err);
+error_free(local_err);
+ret = -EINVAL;
+goto fail;
+}
+
+filename = qemu_opt_get(opts, filename);
+
+client-context = nfs_init_context();
+
+if (client-context == NULL) {
+error_setg(errp, Failed to init NFS context);
+ret = -EINVAL;
+goto fail;
+}
+
+server = g_strdup(filename + 6);


Please check the length of filename is longer than 6 before accessing
filename[6].

Good point. I will check for this, but in fact I think it can't happen
because we will
never end up there if filename does not start with nfs://


True, at least for now, but it doesn't hurt to be defensive,
especially with strings.




+if (server[0] == '/' || server[0] == '\0') {
+error_setg(errp, Invalid server in URL);
+ret = -EINVAL;
+goto fail;
+}
+strp = strchr(server, '/');
+if (strp == NULL) {
+error_setg(errp, Invalid URL specified.\n);
+ret = -EINVAL;
+goto fail;
+}
+path = g_strdup(strp);
+*strp = 0;
+strp = strrchr(path, '/');
+if (strp == NULL) {
+error_setg(errp, Invalid URL specified.\n);
+ret = -EINVAL;
+goto fail;
+}
+file = g_strdup(strp);
+*strp = 0;
+
+if (nfs_mount(client-context, server, path) != 0) {
+error_setg(errp, Failed to mount nfs share: %s,
+nfs_get_error(client-context));
+ret = -EINVAL;
+goto fail;
+}
+
+if (open_flags  O_CREAT) {
+if (nfs_creat(client-context, file, 0600, client-fh)
!= 0) {
+error_setg(errp, Failed to create file: %s,
+ nfs_get_error(client-context));
+ret = -EINVAL;
+goto fail;
+}
+} else {
+open_flags = (flags  BDRV_O_RDWR) ? O_RDWR : O_RDONLY;
+if (nfs_open(client-context, file, open_flags, client-fh)
!= 0) {
+error_setg(errp, Failed to open file : %s,
+ nfs_get_error(client-context));
+ret = -EINVAL;
+goto fail;
+}
+}
+
+if (nfs_fstat(client-context, client-fh, st) != 0) {
+error_setg(errp, Failed to fstat file: %s,
+   nfs_get_error(client-context));
+ret = -EIO;
+goto fail;
+}
+
+bs-total_sectors = st.st_size / BDRV_SECTOR_SIZE;


Please use DIV_ROUND_UP(). Otherwise the remainder in last sector
couldn't be read.

Will do. Can't it happen that we end up reading unallocated sectors?


Hmm, maybe. Not missing last bytes of unaligned sector is essential
for VMDK description file. But you are right, please add check code
and make sure that we don't read beyond EOF as well.

Actually it would like to

Re: [Qemu-devel] [PATCH] block: add native support for NFS

2013-12-17 Thread Fam Zheng


On 2013年12月17日 16:55, Peter Lieven wrote:

On 17.12.2013 09:51, Fam Zheng wrote:

于2013年 12月17日 星期二 16时46分50秒,Peter Lieven写到:

On 17.12.2013 09:29, Fam Zheng wrote:

On 2013年12月17日 15:53, Peter Lieven wrote:

Hi Fam,

On 17.12.2013 05:07, Fam Zheng wrote:

On 2013年12月16日 23:34, Peter Lieven wrote:

+static int coroutine_fn nfs_co_writev(BlockDriverState *bs,
+int64_t sector_num, int
nb_sectors,
+QEMUIOVector *iov)
+{
+nfsclient *client = bs-opaque;
+struct NFSTask Task;
+char *buf = NULL;
+
+nfs_co_init_task(client, Task);
+
+buf = g_malloc(nb_sectors * BDRV_SECTOR_SIZE);
+qemu_iovec_to_buf(iov, 0, buf, nb_sectors * BDRV_SECTOR_SIZE);
+
+if (nfs_pwrite_async(client-context, client-fh,
+ sector_num * BDRV_SECTOR_SIZE,
+ nb_sectors * BDRV_SECTOR_SIZE,
+ buf, nfs_co_generic_cb, Task) != 0) {
+g_free(buf);
+return -EIO;
+}
+
+while (!Task.complete) {
+nfs_set_events(client);
+qemu_coroutine_yield();
+}
+
+g_free(buf);
+
+if (Task.status != nb_sectors * BDRV_SECTOR_SIZE) {
+return -EIO;
+}
+
+bs-total_sectors = MAX(bs-total_sectors, sector_num +
nb_sectors);
+client-allocated_file_size = -ENOTSUP;


Why does allocated_file_size become not supported after a write?

I thought that someone would ask this ;-) bdrv_allocated_file_size
is only
used in image info. I saved some code here implementing an async call.
On open I fstat anyway and store that value. For qemu-img info this is
sufficient, but the allocated size likely changes after a write.
-ENOTSUP
is the default if bdrv_allocated_file_size is not implemented.


OK. Please add some comment here.


+static int nfs_file_open_common(BlockDriverState *bs, QDict
*options, int flags,
+ int open_flags, Error **errp)
+{
+nfsclient *client = bs-opaque;
+const char *filename;
+int ret = 0;
+QemuOpts *opts;
+Error *local_err = NULL;
+char *server = NULL, *path = NULL, *file = NULL, *strp;
+struct stat st;
+
+opts = qemu_opts_create_nofail(runtime_opts);
+qemu_opts_absorb_qdict(opts, options, local_err);
+if (error_is_set(local_err)) {
+qerror_report_err(local_err);
+error_free(local_err);
+ret = -EINVAL;
+goto fail;
+}
+
+filename = qemu_opt_get(opts, filename);
+
+client-context = nfs_init_context();
+
+if (client-context == NULL) {
+error_setg(errp, Failed to init NFS context);
+ret = -EINVAL;
+goto fail;
+}
+
+server = g_strdup(filename + 6);


Please check the length of filename is longer than 6 before accessing
filename[6].

Good point. I will check for this, but in fact I think it can't happen
because we will
never end up there if filename does not start with nfs://


True, at least for now, but it doesn't hurt to be defensive,
especially with strings.




+if (server[0] == '/' || server[0] == '\0') {
+error_setg(errp, Invalid server in URL);
+ret = -EINVAL;
+goto fail;
+}
+strp = strchr(server, '/');
+if (strp == NULL) {
+error_setg(errp, Invalid URL specified.\n);
+ret = -EINVAL;
+goto fail;
+}
+path = g_strdup(strp);
+*strp = 0;
+strp = strrchr(path, '/');
+if (strp == NULL) {
+error_setg(errp, Invalid URL specified.\n);
+ret = -EINVAL;
+goto fail;
+}
+file = g_strdup(strp);
+*strp = 0;
+
+if (nfs_mount(client-context, server, path) != 0) {
+error_setg(errp, Failed to mount nfs share: %s,
+nfs_get_error(client-context));
+ret = -EINVAL;
+goto fail;
+}
+
+if (open_flags  O_CREAT) {
+if (nfs_creat(client-context, file, 0600, client-fh)
!= 0) {
+error_setg(errp, Failed to create file: %s,
+ nfs_get_error(client-context));
+ret = -EINVAL;
+goto fail;
+}
+} else {
+open_flags = (flags  BDRV_O_RDWR) ? O_RDWR : O_RDONLY;
+if (nfs_open(client-context, file, open_flags,
client-fh)
!= 0) {
+error_setg(errp, Failed to open file : %s,
+ nfs_get_error(client-context));
+ret = -EINVAL;
+goto fail;
+}
+}
+
+if (nfs_fstat(client-context, client-fh, st) != 0) {
+error_setg(errp, Failed to fstat file: %s,
+   nfs_get_error(client-context));
+ret = -EIO;
+goto fail;
+}
+
+bs-total_sectors = st.st_size / BDRV_SECTOR_SIZE;


Please use DIV_ROUND_UP(). Otherwise the remainder in last sector
couldn't be read.

Will do. Can't it happen that we end up reading unallocated sectors?


Hmm, maybe. Not missing last bytes of unaligned sector is essential
for VMDK description file. But you are right, please add check code
and make sure that we don't read beyond

Re: [Qemu-devel] [PATCH] block: add native support for NFS

2013-12-17 Thread Peter Lieven


On 17.12.2013 10:01, Fam Zheng wrote:

On 2013年12月17日 16:55, Peter Lieven wrote:

On 17.12.2013 09:51, Fam Zheng wrote:

于2013年 12月17日 星期二 16时46分50秒,Peter Lieven写到:

On 17.12.2013 09:29, Fam Zheng wrote:

On 2013年12月17日 15:53, Peter Lieven wrote:

Hi Fam,

On 17.12.2013 05:07, Fam Zheng wrote:

On 2013年12月16日 23:34, Peter Lieven wrote:

+static int coroutine_fn nfs_co_writev(BlockDriverState *bs,
+int64_t sector_num, int
nb_sectors,
+ QEMUIOVector *iov)
+{
+nfsclient *client = bs-opaque;
+struct NFSTask Task;
+char *buf = NULL;
+
+nfs_co_init_task(client, Task);
+
+buf = g_malloc(nb_sectors * BDRV_SECTOR_SIZE);
+qemu_iovec_to_buf(iov, 0, buf, nb_sectors * BDRV_SECTOR_SIZE);
+
+if (nfs_pwrite_async(client-context, client-fh,
+ sector_num * BDRV_SECTOR_SIZE,
+ nb_sectors * BDRV_SECTOR_SIZE,
+ buf, nfs_co_generic_cb, Task) != 0) {
+g_free(buf);
+return -EIO;
+}
+
+while (!Task.complete) {
+nfs_set_events(client);
+qemu_coroutine_yield();
+}
+
+g_free(buf);
+
+if (Task.status != nb_sectors * BDRV_SECTOR_SIZE) {
+return -EIO;
+}
+
+bs-total_sectors = MAX(bs-total_sectors, sector_num +
nb_sectors);
+client-allocated_file_size = -ENOTSUP;


Why does allocated_file_size become not supported after a write?

I thought that someone would ask this ;-) bdrv_allocated_file_size
is only
used in image info. I saved some code here implementing an async call.
On open I fstat anyway and store that value. For qemu-img info this is
sufficient, but the allocated size likely changes after a write.
-ENOTSUP
is the default if bdrv_allocated_file_size is not implemented.


OK. Please add some comment here.


+static int nfs_file_open_common(BlockDriverState *bs, QDict
*options, int flags,
+ int open_flags, Error **errp)
+{
+nfsclient *client = bs-opaque;
+const char *filename;
+int ret = 0;
+QemuOpts *opts;
+Error *local_err = NULL;
+char *server = NULL, *path = NULL, *file = NULL, *strp;
+struct stat st;
+
+opts = qemu_opts_create_nofail(runtime_opts);
+qemu_opts_absorb_qdict(opts, options, local_err);
+if (error_is_set(local_err)) {
+qerror_report_err(local_err);
+error_free(local_err);
+ret = -EINVAL;
+goto fail;
+}
+
+filename = qemu_opt_get(opts, filename);
+
+client-context = nfs_init_context();
+
+if (client-context == NULL) {
+error_setg(errp, Failed to init NFS context);
+ret = -EINVAL;
+goto fail;
+}
+
+server = g_strdup(filename + 6);


Please check the length of filename is longer than 6 before accessing
filename[6].

Good point. I will check for this, but in fact I think it can't happen
because we will
never end up there if filename does not start with nfs://


True, at least for now, but it doesn't hurt to be defensive,
especially with strings.




+if (server[0] == '/' || server[0] == '\0') {
+error_setg(errp, Invalid server in URL);
+ret = -EINVAL;
+goto fail;
+}
+strp = strchr(server, '/');
+if (strp == NULL) {
+error_setg(errp, Invalid URL specified.\n);
+ret = -EINVAL;
+goto fail;
+}
+path = g_strdup(strp);
+*strp = 0;
+strp = strrchr(path, '/');
+if (strp == NULL) {
+error_setg(errp, Invalid URL specified.\n);
+ret = -EINVAL;
+goto fail;
+}
+file = g_strdup(strp);
+*strp = 0;
+
+if (nfs_mount(client-context, server, path) != 0) {
+error_setg(errp, Failed to mount nfs share: %s,
+ nfs_get_error(client-context));
+ret = -EINVAL;
+goto fail;
+}
+
+if (open_flags  O_CREAT) {
+if (nfs_creat(client-context, file, 0600, client-fh)
!= 0) {
+error_setg(errp, Failed to create file: %s,
+ nfs_get_error(client-context));
+ret = -EINVAL;
+goto fail;
+}
+} else {
+open_flags = (flags  BDRV_O_RDWR) ? O_RDWR : O_RDONLY;
+if (nfs_open(client-context, file, open_flags,
client-fh)
!= 0) {
+error_setg(errp, Failed to open file : %s,
+ nfs_get_error(client-context));
+ret = -EINVAL;
+goto fail;
+}
+}
+
+if (nfs_fstat(client-context, client-fh, st) != 0) {
+error_setg(errp, Failed to fstat file: %s,
+ nfs_get_error(client-context));
+ret = -EIO;
+goto fail;
+}
+
+bs-total_sectors = st.st_size / BDRV_SECTOR_SIZE;


Please use DIV_ROUND_UP(). Otherwise the remainder in last sector
couldn't be read.

Will do. Can't it happen that we end up reading unallocated sectors?


Hmm, maybe. Not missing last bytes of unaligned sector is essential
for VMDK description file. But you are right, please add check code
and make sure that we don't read beyond EOF as well.

Actually it would like

Re: [Qemu-devel] [PATCH] block: add native support for NFS

2013-12-17 Thread Fam Zheng


On 2013年12月17日 17:07, Peter Lieven wrote:

On 17.12.2013 10:01, Fam Zheng wrote:

On 2013年12月17日 16:55, Peter Lieven wrote:

On 17.12.2013 09:51, Fam Zheng wrote:

于2013年 12月17日 星期二 16时46分50秒,Peter Lieven写到:

On 17.12.2013 09:29, Fam Zheng wrote:

On 2013年12月17日 15:53, Peter Lieven wrote:

Hi Fam,

On 17.12.2013 05:07, Fam Zheng wrote:

On 2013年12月16日 23:34, Peter Lieven wrote:

+static int coroutine_fn nfs_co_writev(BlockDriverState *bs,
+int64_t sector_num, int
nb_sectors,
+ QEMUIOVector *iov)
+{
+nfsclient *client = bs-opaque;
+struct NFSTask Task;
+char *buf = NULL;
+
+nfs_co_init_task(client, Task);
+
+buf = g_malloc(nb_sectors * BDRV_SECTOR_SIZE);
+qemu_iovec_to_buf(iov, 0, buf, nb_sectors *
BDRV_SECTOR_SIZE);
+
+if (nfs_pwrite_async(client-context, client-fh,
+ sector_num * BDRV_SECTOR_SIZE,
+ nb_sectors * BDRV_SECTOR_SIZE,
+ buf, nfs_co_generic_cb, Task) != 0) {
+g_free(buf);
+return -EIO;
+}
+
+while (!Task.complete) {
+nfs_set_events(client);
+qemu_coroutine_yield();
+}
+
+g_free(buf);
+
+if (Task.status != nb_sectors * BDRV_SECTOR_SIZE) {
+return -EIO;
+}
+
+bs-total_sectors = MAX(bs-total_sectors, sector_num +
nb_sectors);
+client-allocated_file_size = -ENOTSUP;


Why does allocated_file_size become not supported after a write?

I thought that someone would ask this ;-) bdrv_allocated_file_size
is only
used in image info. I saved some code here implementing an async
call.
On open I fstat anyway and store that value. For qemu-img info
this is
sufficient, but the allocated size likely changes after a write.
-ENOTSUP
is the default if bdrv_allocated_file_size is not implemented.


OK. Please add some comment here.


+static int nfs_file_open_common(BlockDriverState *bs, QDict
*options, int flags,
+ int open_flags, Error **errp)
+{
+nfsclient *client = bs-opaque;
+const char *filename;
+int ret = 0;
+QemuOpts *opts;
+Error *local_err = NULL;
+char *server = NULL, *path = NULL, *file = NULL, *strp;
+struct stat st;
+
+opts = qemu_opts_create_nofail(runtime_opts);
+qemu_opts_absorb_qdict(opts, options, local_err);
+if (error_is_set(local_err)) {
+qerror_report_err(local_err);
+error_free(local_err);
+ret = -EINVAL;
+goto fail;
+}
+
+filename = qemu_opt_get(opts, filename);
+
+client-context = nfs_init_context();
+
+if (client-context == NULL) {
+error_setg(errp, Failed to init NFS context);
+ret = -EINVAL;
+goto fail;
+}
+
+server = g_strdup(filename + 6);


Please check the length of filename is longer than 6 before
accessing
filename[6].

Good point. I will check for this, but in fact I think it can't
happen
because we will
never end up there if filename does not start with nfs://


True, at least for now, but it doesn't hurt to be defensive,
especially with strings.




+if (server[0] == '/' || server[0] == '\0') {
+error_setg(errp, Invalid server in URL);
+ret = -EINVAL;
+goto fail;
+}
+strp = strchr(server, '/');
+if (strp == NULL) {
+error_setg(errp, Invalid URL specified.\n);
+ret = -EINVAL;
+goto fail;
+}
+path = g_strdup(strp);
+*strp = 0;
+strp = strrchr(path, '/');
+if (strp == NULL) {
+error_setg(errp, Invalid URL specified.\n);
+ret = -EINVAL;
+goto fail;
+}
+file = g_strdup(strp);
+*strp = 0;
+
+if (nfs_mount(client-context, server, path) != 0) {
+error_setg(errp, Failed to mount nfs share: %s,
+ nfs_get_error(client-context));
+ret = -EINVAL;
+goto fail;
+}
+
+if (open_flags  O_CREAT) {
+if (nfs_creat(client-context, file, 0600, client-fh)
!= 0) {
+error_setg(errp, Failed to create file: %s,
+ nfs_get_error(client-context));
+ret = -EINVAL;
+goto fail;
+}
+} else {
+open_flags = (flags  BDRV_O_RDWR) ? O_RDWR : O_RDONLY;
+if (nfs_open(client-context, file, open_flags,
client-fh)
!= 0) {
+error_setg(errp, Failed to open file : %s,
+ nfs_get_error(client-context));
+ret = -EINVAL;
+goto fail;
+}
+}
+
+if (nfs_fstat(client-context, client-fh, st) != 0) {
+error_setg(errp, Failed to fstat file: %s,
+ nfs_get_error(client-context));
+ret = -EIO;
+goto fail;
+}
+
+bs-total_sectors = st.st_size / BDRV_SECTOR_SIZE;


Please use DIV_ROUND_UP(). Otherwise the remainder in last sector
couldn't be read.

Will do. Can't it happen that we end up reading unallocated sectors?


Hmm, maybe. Not missing last bytes of unaligned sector is essential
for VMDK description file. But you are right, please add check code
and make sure that we don't read

Re: [Qemu-devel] [PATCH] block: add native support for NFS

2013-12-17 Thread Peter Lieven


On 17.12.2013 10:46, Fam Zheng wrote:

On 2013年12月17日 17:07, Peter Lieven wrote:

On 17.12.2013 10:01, Fam Zheng wrote:

On 2013年12月17日 16:55, Peter Lieven wrote:

On 17.12.2013 09:51, Fam Zheng wrote:

于2013年 12月17日 星期二 16时46分50秒,Peter Lieven写到:

On 17.12.2013 09:29, Fam Zheng wrote:

On 2013年12月17日 15:53, Peter Lieven wrote:

Hi Fam,

On 17.12.2013 05:07, Fam Zheng wrote:

On 2013年12月16日 23:34, Peter Lieven wrote:

+static int coroutine_fn nfs_co_writev(BlockDriverState *bs,
+int64_t sector_num, int
nb_sectors,
+ QEMUIOVector *iov)
+{
+nfsclient *client = bs-opaque;
+struct NFSTask Task;
+char *buf = NULL;
+
+nfs_co_init_task(client, Task);
+
+buf = g_malloc(nb_sectors * BDRV_SECTOR_SIZE);
+qemu_iovec_to_buf(iov, 0, buf, nb_sectors *
BDRV_SECTOR_SIZE);
+
+if (nfs_pwrite_async(client-context, client-fh,
+ sector_num * BDRV_SECTOR_SIZE,
+ nb_sectors * BDRV_SECTOR_SIZE,
+ buf, nfs_co_generic_cb, Task) != 0) {
+g_free(buf);
+return -EIO;
+}
+
+while (!Task.complete) {
+nfs_set_events(client);
+qemu_coroutine_yield();
+}
+
+g_free(buf);
+
+if (Task.status != nb_sectors * BDRV_SECTOR_SIZE) {
+return -EIO;
+}
+
+bs-total_sectors = MAX(bs-total_sectors, sector_num +
nb_sectors);
+client-allocated_file_size = -ENOTSUP;


Why does allocated_file_size become not supported after a write?

I thought that someone would ask this ;-) bdrv_allocated_file_size
is only
used in image info. I saved some code here implementing an async
call.
On open I fstat anyway and store that value. For qemu-img info
this is
sufficient, but the allocated size likely changes after a write.
-ENOTSUP
is the default if bdrv_allocated_file_size is not implemented.


OK. Please add some comment here.


+static int nfs_file_open_common(BlockDriverState *bs, QDict
*options, int flags,
+ int open_flags, Error **errp)
+{
+nfsclient *client = bs-opaque;
+const char *filename;
+int ret = 0;
+QemuOpts *opts;
+Error *local_err = NULL;
+char *server = NULL, *path = NULL, *file = NULL, *strp;
+struct stat st;
+
+opts = qemu_opts_create_nofail(runtime_opts);
+qemu_opts_absorb_qdict(opts, options, local_err);
+if (error_is_set(local_err)) {
+qerror_report_err(local_err);
+error_free(local_err);
+ret = -EINVAL;
+goto fail;
+}
+
+filename = qemu_opt_get(opts, filename);
+
+client-context = nfs_init_context();
+
+if (client-context == NULL) {
+error_setg(errp, Failed to init NFS context);
+ret = -EINVAL;
+goto fail;
+}
+
+server = g_strdup(filename + 6);


Please check the length of filename is longer than 6 before
accessing
filename[6].

Good point. I will check for this, but in fact I think it can't
happen
because we will
never end up there if filename does not start with nfs://


True, at least for now, but it doesn't hurt to be defensive,
especially with strings.




+if (server[0] == '/' || server[0] == '\0') {
+error_setg(errp, Invalid server in URL);
+ret = -EINVAL;
+goto fail;
+}
+strp = strchr(server, '/');
+if (strp == NULL) {
+error_setg(errp, Invalid URL specified.\n);
+ret = -EINVAL;
+goto fail;
+}
+path = g_strdup(strp);
+*strp = 0;
+strp = strrchr(path, '/');
+if (strp == NULL) {
+error_setg(errp, Invalid URL specified.\n);
+ret = -EINVAL;
+goto fail;
+}
+file = g_strdup(strp);
+*strp = 0;
+
+if (nfs_mount(client-context, server, path) != 0) {
+error_setg(errp, Failed to mount nfs share: %s,
+ nfs_get_error(client-context));
+ret = -EINVAL;
+goto fail;
+}
+
+if (open_flags  O_CREAT) {
+if (nfs_creat(client-context, file, 0600, client-fh)
!= 0) {
+error_setg(errp, Failed to create file: %s,
+ nfs_get_error(client-context));
+ret = -EINVAL;
+goto fail;
+}
+} else {
+open_flags = (flags  BDRV_O_RDWR) ? O_RDWR : O_RDONLY;
+if (nfs_open(client-context, file, open_flags,
client-fh)
!= 0) {
+error_setg(errp, Failed to open file : %s,
+ nfs_get_error(client-context));
+ret = -EINVAL;
+goto fail;
+}
+}
+
+if (nfs_fstat(client-context, client-fh, st) != 0) {
+error_setg(errp, Failed to fstat file: %s,
+ nfs_get_error(client-context));
+ret = -EIO;
+goto fail;
+}
+
+bs-total_sectors = st.st_size / BDRV_SECTOR_SIZE;


Please use DIV_ROUND_UP(). Otherwise the remainder in last sector
couldn't be read.

Will do. Can't it happen that we end up reading unallocated sectors?


Hmm, maybe. Not missing last bytes of unaligned sector is essential
for VMDK description file. But you are right, please add check

Re: [Qemu-devel] [PATCH] block: add native support for NFS

2013-12-16 Thread ronnie sahlberg

Nice.

+block-obj-$(CONFIG_LIBISCSI) += nfs.

Should be CONFIG_LIBNFS

On Mon, Dec 16, 2013 at 7:34 AM, Peter Lieven p...@kamp.de wrote:
 This patch adds native support for accessing images on NFS shares without
 the requirement to actually mount the entire NFS share on the host.

 NFS Images can simply be specified by an url of the form:
 nfs://host/export/filename

 For example:
 qemu-img create -f qcow2 nfs://10.0.0.1/qemu-images/test.qcow2

 You need libnfs from Ronnie Sahlberg available at:
git://github.com/sahlberg/libnfs.git
 for this to work.

 During configure it is automatically probed for libnfs and support
 is enabled on-the-fly. You can forbid or enforce libnfs support
 with --disable-libnfs or --enable-libnfs respectively.

 Due to NFS restrictions you might need to execute your binaries
 as root, allow them to open priviledged ports (1024) or specify
 insecure option on the NFS server.

 Signed-off-by: Peter Lieven p...@kamp.de
 ---
  MAINTAINERS |5 +
  block/Makefile.objs |1 +
  block/nfs.c |  420 
 +++
  configure   |   38 +
  4 files changed, 464 insertions(+)
  create mode 100644 block/nfs.c

 diff --git a/MAINTAINERS b/MAINTAINERS
 index c19133f..f53d184 100644
 --- a/MAINTAINERS
 +++ b/MAINTAINERS
 @@ -899,6 +899,11 @@ M: Peter Lieven p...@kamp.de
  S: Supported
  F: block/iscsi.c

 +NFS
 +M: Peter Lieven p...@kamp.de
 +S: Maintained
 +F: block/nfs.c
 +
  SSH
  M: Richard W.M. Jones rjo...@redhat.com
  S: Supported
 diff --git a/block/Makefile.objs b/block/Makefile.objs
 index f43ecbc..1bac94e 100644
 --- a/block/Makefile.objs
 +++ b/block/Makefile.objs
 @@ -12,6 +12,7 @@ block-obj-$(CONFIG_LINUX_AIO) += linux-aio.o
  ifeq ($(CONFIG_POSIX),y)
  block-obj-y += nbd.o sheepdog.o
  block-obj-$(CONFIG_LIBISCSI) += iscsi.o
 +block-obj-$(CONFIG_LIBISCSI) += nfs.o
  block-obj-$(CONFIG_CURL) += curl.o
  block-obj-$(CONFIG_RBD) += rbd.o
  block-obj-$(CONFIG_GLUSTERFS) += gluster.o
 diff --git a/block/nfs.c b/block/nfs.c
 new file mode 100644
 index 000..d6cb4c0
 --- /dev/null
 +++ b/block/nfs.c
 @@ -0,0 +1,420 @@
 +/*
 + * QEMU Block driver for native access to files on NFS shares
 + *
 + * Copyright (c) 2013 Peter Lieven p...@kamp.de
 + *
 + * Permission is hereby granted, free of charge, to any person obtaining a 
 copy
 + * of this software and associated documentation files (the Software), to 
 deal
 + * in the Software without restriction, including without limitation the 
 rights
 + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
 + * copies of the Software, and to permit persons to whom the Software is
 + * furnished to do so, subject to the following conditions:
 + *
 + * The above copyright notice and this permission notice shall be included in
 + * all copies or substantial portions of the Software.
 + *
 + * THE SOFTWARE IS PROVIDED AS IS, WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
 + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
 + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
 + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
 + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING 
 FROM,
 + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
 + * THE SOFTWARE.
 + */
 +
 +#include config-host.h
 +
 +#include poll.h
 +#include arpa/inet.h
 +#include qemu-common.h
 +#include qemu/config-file.h
 +#include qemu/error-report.h
 +#include block/block_int.h
 +#include trace.h
 +#include block/scsi.h
 +#include qemu/iov.h
 +#include sysemu/sysemu.h
 +#include qmp-commands.h
 +
 +#include nfsc/libnfs-zdr.h
 +#include nfsc/libnfs.h
 +#include nfsc/libnfs-raw.h
 +#include nfsc/libnfs-raw-mount.h
 +
 +typedef struct nfsclient {
 +struct nfs_context *context;
 +struct nfsfh *fh;
 +int events;
 +bool has_zero_init;
 +int64_t allocated_file_size;
 +QEMUBH *close_bh;
 +} nfsclient;
 +
 +typedef struct NFSTask {
 +int status;
 +int complete;
 +QEMUIOVector *iov;
 +Coroutine *co;
 +QEMUBH *bh;
 +} NFSTask;
 +
 +static void nfs_process_read(void *arg);
 +static void nfs_process_write(void *arg);
 +
 +static void nfs_set_events(nfsclient *client)
 +{
 +int ev;
 +/* We always register a read handler.  */
 +ev = POLLIN;
 +ev |= nfs_which_events(client-context);
 +if (ev != client-events) {
 +qemu_aio_set_fd_handler(nfs_get_fd(client-context),
 +  nfs_process_read,
 +  (ev  POLLOUT) ? nfs_process_write : NULL,
 +  client);
 +
 +}
 +client-events = ev;
 +}
 +
 +static void nfs_process_read(void *arg)
 +{
 +nfsclient *client = arg;
 +nfs_service(client-context, POLLIN);
 +nfs_set_events(client);
 +}
 +
 +static void nfs_process_write(void *arg)
 +{
 +nfsclient *client = arg;
 +nfs_service(client-context,

Re: [Qemu-devel] [PATCH] block: add native support for NFS

2013-12-16 Thread ronnie sahlberg

On Mon, Dec 16, 2013 at 7:34 AM, Peter Lieven p...@kamp.de wrote:
 This patch adds native support for accessing images on NFS shares without
 the requirement to actually mount the entire NFS share on the host.

 NFS Images can simply be specified by an url of the form:
 nfs://host/export/filename

 For example:
 qemu-img create -f qcow2 nfs://10.0.0.1/qemu-images/test.qcow2

 You need libnfs from Ronnie Sahlberg available at:
git://github.com/sahlberg/libnfs.git
 for this to work.

 During configure it is automatically probed for libnfs and support
 is enabled on-the-fly. You can forbid or enforce libnfs support
 with --disable-libnfs or --enable-libnfs respectively.

 Due to NFS restrictions you might need to execute your binaries
 as root, allow them to open priviledged ports (1024) or specify
 insecure option on the NFS server.

 Signed-off-by: Peter Lieven p...@kamp.de
 ---
  MAINTAINERS |5 +
  block/Makefile.objs |1 +
  block/nfs.c |  420 
 +++
  configure   |   38 +
  4 files changed, 464 insertions(+)
  create mode 100644 block/nfs.c

 diff --git a/MAINTAINERS b/MAINTAINERS
 index c19133f..f53d184 100644
 --- a/MAINTAINERS
 +++ b/MAINTAINERS
 @@ -899,6 +899,11 @@ M: Peter Lieven p...@kamp.de
  S: Supported
  F: block/iscsi.c

 +NFS
 +M: Peter Lieven p...@kamp.de
 +S: Maintained
 +F: block/nfs.c
 +
  SSH
  M: Richard W.M. Jones rjo...@redhat.com
  S: Supported
 diff --git a/block/Makefile.objs b/block/Makefile.objs
 index f43ecbc..1bac94e 100644
 --- a/block/Makefile.objs
 +++ b/block/Makefile.objs
 @@ -12,6 +12,7 @@ block-obj-$(CONFIG_LINUX_AIO) += linux-aio.o
  ifeq ($(CONFIG_POSIX),y)
  block-obj-y += nbd.o sheepdog.o
  block-obj-$(CONFIG_LIBISCSI) += iscsi.o
 +block-obj-$(CONFIG_LIBISCSI) += nfs.o

CONFIG_LIBNFS

  block-obj-$(CONFIG_CURL) += curl.o
  block-obj-$(CONFIG_RBD) += rbd.o
  block-obj-$(CONFIG_GLUSTERFS) += gluster.o
 diff --git a/block/nfs.c b/block/nfs.c
 new file mode 100644
 index 000..d6cb4c0
 --- /dev/null
 +++ b/block/nfs.c
 @@ -0,0 +1,420 @@
 +/*
 + * QEMU Block driver for native access to files on NFS shares
 + *
 + * Copyright (c) 2013 Peter Lieven p...@kamp.de
 + *
 + * Permission is hereby granted, free of charge, to any person obtaining a 
 copy
 + * of this software and associated documentation files (the Software), to 
 deal
 + * in the Software without restriction, including without limitation the 
 rights
 + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
 + * copies of the Software, and to permit persons to whom the Software is
 + * furnished to do so, subject to the following conditions:
 + *
 + * The above copyright notice and this permission notice shall be included in
 + * all copies or substantial portions of the Software.
 + *
 + * THE SOFTWARE IS PROVIDED AS IS, WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
 + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
 + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
 + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
 + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING 
 FROM,
 + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
 + * THE SOFTWARE.
 + */
 +
 +#include config-host.h
 +
 +#include poll.h
 +#include arpa/inet.h
 +#include qemu-common.h
 +#include qemu/config-file.h
 +#include qemu/error-report.h
 +#include block/block_int.h
 +#include trace.h
 +#include block/scsi.h
 +#include qemu/iov.h
 +#include sysemu/sysemu.h
 +#include qmp-commands.h
 +
 +#include nfsc/libnfs-zdr.h
 +#include nfsc/libnfs.h
 +#include nfsc/libnfs-raw.h
 +#include nfsc/libnfs-raw-mount.h
 +
 +typedef struct nfsclient {
 +struct nfs_context *context;
 +struct nfsfh *fh;
 +int events;
 +bool has_zero_init;
 +int64_t allocated_file_size;
 +QEMUBH *close_bh;
 +} nfsclient;
 +
 +typedef struct NFSTask {
 +int status;
 +int complete;
 +QEMUIOVector *iov;
 +Coroutine *co;
 +QEMUBH *bh;
 +} NFSTask;
 +
 +static void nfs_process_read(void *arg);
 +static void nfs_process_write(void *arg);
 +
 +static void nfs_set_events(nfsclient *client)
 +{
 +int ev;
 +/* We always register a read handler.  */
 +ev = POLLIN;

I don't think you should always register a read handler here since
there are no situations where the NFS server
will send unsolicited packets back to the client (contrary to iSCSI
where the target might send NOPs back to ping the client).

When NFS sessions are idle, it is common that servers will tear down
the TCP connection to save resources and rely on that
the clients will transparently reconnect the TPC session again once
there is new I/O to send.
This reconnect is handled automatically/transparently within libnfs.

 +ev |= nfs_which_events(client-context);
 +if (ev != client-events) {
 +

Re: [Qemu-devel] [PATCH] block: add native support for NFS

2013-12-16 Thread Fam Zheng


On 2013年12月16日 23:34, Peter Lieven wrote:

This patch adds native support for accessing images on NFS shares without
the requirement to actually mount the entire NFS share on the host.

NFS Images can simply be specified by an url of the form:
nfs://host/export/filename

For example:
qemu-img create -f qcow2 nfs://10.0.0.1/qemu-images/test.qcow2

You need libnfs from Ronnie Sahlberg available at:
git://github.com/sahlberg/libnfs.git
for this to work.

During configure it is automatically probed for libnfs and support
is enabled on-the-fly. You can forbid or enforce libnfs support
with --disable-libnfs or --enable-libnfs respectively.

Due to NFS restrictions you might need to execute your binaries
as root, allow them to open priviledged ports (1024) or specify
insecure option on the NFS server.

Signed-off-by: Peter Lieven p...@kamp.de


Looks nice! Thanks for the work!


---
  MAINTAINERS |5 +
  block/Makefile.objs |1 +
  block/nfs.c |  420 +++
  configure   |   38 +
  4 files changed, 464 insertions(+)
  create mode 100644 block/nfs.c

diff --git a/MAINTAINERS b/MAINTAINERS
index c19133f..f53d184 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -899,6 +899,11 @@ M: Peter Lieven p...@kamp.de
  S: Supported
  F: block/iscsi.c

+NFS
+M: Peter Lieven p...@kamp.de
+S: Maintained
+F: block/nfs.c
+
  SSH
  M: Richard W.M. Jones rjo...@redhat.com
  S: Supported
diff --git a/block/Makefile.objs b/block/Makefile.objs
index f43ecbc..1bac94e 100644
--- a/block/Makefile.objs
+++ b/block/Makefile.objs
@@ -12,6 +12,7 @@ block-obj-$(CONFIG_LINUX_AIO) += linux-aio.o
  ifeq ($(CONFIG_POSIX),y)
  block-obj-y += nbd.o sheepdog.o
  block-obj-$(CONFIG_LIBISCSI) += iscsi.o
+block-obj-$(CONFIG_LIBISCSI) += nfs.o
  block-obj-$(CONFIG_CURL) += curl.o
  block-obj-$(CONFIG_RBD) += rbd.o
  block-obj-$(CONFIG_GLUSTERFS) += gluster.o
diff --git a/block/nfs.c b/block/nfs.c
new file mode 100644
index 000..d6cb4c0
--- /dev/null
+++ b/block/nfs.c
@@ -0,0 +1,420 @@
+/*
+ * QEMU Block driver for native access to files on NFS shares
+ *
+ * Copyright (c) 2013 Peter Lieven p...@kamp.de
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the Software), to 
deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED AS IS, WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING 
FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#include config-host.h
+
+#include poll.h
+#include arpa/inet.h
+#include qemu-common.h
+#include qemu/config-file.h
+#include qemu/error-report.h
+#include block/block_int.h
+#include trace.h
+#include block/scsi.h
+#include qemu/iov.h
+#include sysemu/sysemu.h
+#include qmp-commands.h


Copied from block/iscsi.c? SCSI and QMP are not necessary for this file. 
And maybe also arpa/inet.h, I'm not sure about that though.



+
+#include nfsc/libnfs-zdr.h
+#include nfsc/libnfs.h
+#include nfsc/libnfs-raw.h
+#include nfsc/libnfs-raw-mount.h
+
+typedef struct nfsclient {
+struct nfs_context *context;
+struct nfsfh *fh;
+int events;
+bool has_zero_init;
+int64_t allocated_file_size;
+QEMUBH *close_bh;


This is unused.


+} nfsclient;


Please use CamelCase for type names...


+
+typedef struct NFSTask {
+int status;
+int complete;
+QEMUIOVector *iov;
+Coroutine *co;
+QEMUBH *bh;
+} NFSTask;


as you do with this.


+
+static void nfs_process_read(void *arg);
+static void nfs_process_write(void *arg);
+
+static void nfs_set_events(nfsclient *client)
+{
+int ev;
+/* We always register a read handler.  */
+ev = POLLIN;
+ev |= nfs_which_events(client-context);
+if (ev != client-events) {
+qemu_aio_set_fd_handler(nfs_get_fd(client-context),
+  nfs_process_read,
+  (ev  POLLOUT) ? nfs_process_write : NULL,
+  client);
+
+}
+client-events = ev;
+}
+
+static void nfs_process_read(void *arg)
+{
+nfsclient *client = arg;
+nfs_service(client-context, POLLIN);
+nfs_set_events(client);
+}
+
+static void nfs_process_write(void *arg)
+{
+nfsclient *client =

Re: [Qemu-devel] [PATCH] block: add native support for NFS

2013-12-16 Thread Peter Lieven


Hi Fam,

On 17.12.2013 05:07, Fam Zheng wrote:

On 2013年12月16日 23:34, Peter Lieven wrote:

This patch adds native support for accessing images on NFS shares without
the requirement to actually mount the entire NFS share on the host.

NFS Images can simply be specified by an url of the form:
nfs://host/export/filename

For example:
qemu-img create -f qcow2 nfs://10.0.0.1/qemu-images/test.qcow2

You need libnfs from Ronnie Sahlberg available at:
git://github.com/sahlberg/libnfs.git
for this to work.

During configure it is automatically probed for libnfs and support
is enabled on-the-fly. You can forbid or enforce libnfs support
with --disable-libnfs or --enable-libnfs respectively.

Due to NFS restrictions you might need to execute your binaries
as root, allow them to open priviledged ports (1024) or specify
insecure option on the NFS server.

Signed-off-by: Peter Lieven p...@kamp.de


Looks nice! Thanks for the work!

Thank you ;-)



---
  MAINTAINERS |5 +
  block/Makefile.objs |1 +
  block/nfs.c |  420 +++
  configure   |   38 +
  4 files changed, 464 insertions(+)
  create mode 100644 block/nfs.c

diff --git a/MAINTAINERS b/MAINTAINERS
index c19133f..f53d184 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -899,6 +899,11 @@ M: Peter Lieven p...@kamp.de
  S: Supported
  F: block/iscsi.c

+NFS
+M: Peter Lieven p...@kamp.de
+S: Maintained
+F: block/nfs.c
+
  SSH
  M: Richard W.M. Jones rjo...@redhat.com
  S: Supported
diff --git a/block/Makefile.objs b/block/Makefile.objs
index f43ecbc..1bac94e 100644
--- a/block/Makefile.objs
+++ b/block/Makefile.objs
@@ -12,6 +12,7 @@ block-obj-$(CONFIG_LINUX_AIO) += linux-aio.o
  ifeq ($(CONFIG_POSIX),y)
  block-obj-y += nbd.o sheepdog.o
  block-obj-$(CONFIG_LIBISCSI) += iscsi.o
+block-obj-$(CONFIG_LIBISCSI) += nfs.o
  block-obj-$(CONFIG_CURL) += curl.o
  block-obj-$(CONFIG_RBD) += rbd.o
  block-obj-$(CONFIG_GLUSTERFS) += gluster.o
diff --git a/block/nfs.c b/block/nfs.c
new file mode 100644
index 000..d6cb4c0
--- /dev/null
+++ b/block/nfs.c
@@ -0,0 +1,420 @@
+/*
+ * QEMU Block driver for native access to files on NFS shares
+ *
+ * Copyright (c) 2013 Peter Lieven p...@kamp.de
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the Software), to 
deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED AS IS, WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING 
FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#include config-host.h
+
+#include poll.h
+#include arpa/inet.h
+#include qemu-common.h
+#include qemu/config-file.h
+#include qemu/error-report.h
+#include block/block_int.h
+#include trace.h
+#include block/scsi.h
+#include qemu/iov.h
+#include sysemu/sysemu.h
+#include qmp-commands.h


Copied from block/iscsi.c? SCSI and QMP are not necessary for this file. And 
maybe also arpa/inet.h, I'm not sure about that though.

Yes, libiscsi and libnfs are quite similar. ;-)



+
+#include nfsc/libnfs-zdr.h
+#include nfsc/libnfs.h
+#include nfsc/libnfs-raw.h
+#include nfsc/libnfs-raw-mount.h
+
+typedef struct nfsclient {
+struct nfs_context *context;
+struct nfsfh *fh;
+int events;
+bool has_zero_init;
+int64_t allocated_file_size;
+QEMUBH *close_bh;


This is unused.

Ups, its a leftover from a nasty segfault debug session.



+} nfsclient;


Please use CamelCase for type names...

Ok



+
+typedef struct NFSTask {
+int status;
+int complete;
+QEMUIOVector *iov;
+Coroutine *co;
+QEMUBH *bh;
+} NFSTask;


as you do with this.


+
+static void nfs_process_read(void *arg);
+static void nfs_process_write(void *arg);
+
+static void nfs_set_events(nfsclient *client)
+{
+int ev;
+/* We always register a read handler.  */
+ev = POLLIN;
+ev |= nfs_which_events(client-context);
+if (ev != client-events) {
+qemu_aio_set_fd_handler(nfs_get_fd(client-context),
+  nfs_process_read,
+  (ev  POLLOUT) ? nfs_process_write : NULL,
+  client);
+
+}
+client-events = ev;
+}
+
+static void nfs_process_read(void *arg)
+{
+

Re: [Qemu-devel] [PATCH] block: add native support for NFS

Re: [Qemu-devel] [PATCH] block: add native support for NFS

Re: [Qemu-devel] [PATCH] block: add native support for NFS

Re: [Qemu-devel] [PATCH] block: add native support for NFS

Re: [Qemu-devel] [PATCH] block: add native support for NFS

Re: [Qemu-devel] [PATCH] block: add native support for NFS

Re: [Qemu-devel] [PATCH] block: add native support for NFS

Re: [Qemu-devel] [PATCH] block: add native support for NFS

Re: [Qemu-devel] [PATCH] block: add native support for NFS

Re: [Qemu-devel] [PATCH] block: add native support for NFS

Re: [Qemu-devel] [PATCH] block: add native support for NFS

Re: [Qemu-devel] [PATCH] block: add native support for NFS

12 matches

Site Navigation

Mail list logo

Footer information