Re: [PATCH v5 2/4] block: add the block queue support

2011-08-09 Thread Ram Pai 
On Tue, Aug 09, 2011 at 12:17:50PM +0800, Zhi Yong Wu wrote:
 The patch introduce one block queue for QEMU block layer.
 
 Signed-off-by: Zhi Yong Wu wu...@linux.vnet.ibm.com
 ---
  block/blk-queue.c |  141 
 +
  block/blk-queue.h |   73 +++
  2 files changed, 214 insertions(+), 0 deletions(-)
  create mode 100644 block/blk-queue.c
  create mode 100644 block/blk-queue.h
 
 diff --git a/block/blk-queue.c b/block/blk-queue.c
 new file mode 100644
 index 000..f36f3e2
 --- /dev/null
 +++ b/block/blk-queue.c
 @@ -0,0 +1,141 @@
 +/*
 + * QEMU System Emulator queue definition for block layer
 + *
 + * Copyright (c) 2011 Zhi Yong Wu  zwu.ker...@gmail.com
 + *
 + * 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 block_int.h
 +#include qemu-queue.h
 +#include block/blk-queue.h
 +
 +/* The APIs for block request queue on qemu block layer.
 + */
 +
 +static void qemu_block_queue_cancel(BlockDriverAIOCB *acb)
 +{
 +qemu_aio_release(acb);
 +}
 +
 +static AIOPool block_queue_pool = {
 +.aiocb_size = sizeof(struct BlockDriverAIOCB),
 +.cancel = qemu_block_queue_cancel,
 +};
 +
 +static void qemu_block_queue_callback(void *opaque, int ret)
 +{
 +BlockDriverAIOCB *acb = opaque;
 +
 +qemu_aio_release(acb);
 +}
 +
 +BlockQueue *qemu_new_block_queue(void)
 +{
 +BlockQueue *queue;
 +
 +queue = qemu_mallocz(sizeof(BlockQueue));
 +
 +QTAILQ_INIT(queue-requests);
 +
 +return queue;
 +}
 +
 +void qemu_del_block_queue(BlockQueue *queue)
 +{
 +BlockIORequest *request, *next;
 +
 +QTAILQ_FOREACH_SAFE(request, queue-requests, entry, next) {
 +QTAILQ_REMOVE(queue-requests, request, entry);
 +qemu_free(request);
 +}
 +
 +qemu_free(queue);
 +}
 +
 +BlockDriverAIOCB *qemu_block_queue_enqueue(BlockQueue *queue,
 +BlockDriverState *bs,
 +BlockRequestHandler *handler,
 +int64_t sector_num,
 +QEMUIOVector *qiov,
 +int nb_sectors,
 +BlockDriverCompletionFunc *cb,
 +void *opaque)
 +{
 +BlockIORequest *request;
 +BlockDriverAIOCB *acb;
 +
 +request = qemu_malloc(sizeof(BlockIORequest));
 +request-bs = bs;
 +request-handler = handler;
 +request-sector_num = sector_num;
 +request-qiov = qiov;
 +request-nb_sectors = nb_sectors;
 +request-cb = cb;
 +request-opaque = opaque;
 +
 +QTAILQ_INSERT_TAIL(queue-requests, request, entry);
 +
 +acb = qemu_aio_get(block_queue_pool, bs,
 +   qemu_block_queue_callback, opaque);
 +
 +request-acb = acb;
 +
 +return acb;
 +}
 +
 +int qemu_block_queue_handler(BlockIORequest *request)
 +{
 +int ret;
 +BlockDriverAIOCB *res;
 +
 +/* indicate this req is from block queue */
 +request-bs-req_from_queue = true;
 +
 +res = request-handler(request-bs, request-sector_num,
 +   request-qiov, request-nb_sectors,
 +   request-cb, request-opaque);


should'nt you return with a failure here if res == NULL?
Otherwise  qemu_block_queue_callback() gets called which 
will release the acb.


 +
 +if (request-acb) {
 +qemu_block_queue_callback(request-acb, 0);
 +}
 +
 +ret = (res == NULL) ? 0 : 1;
 +
 +return ret;
 +}


RP
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 3/4] block: add block timer and block throttling algorithm

2011-08-09 Thread Ram Pai 
On Tue, Aug 09, 2011 at 12:17:51PM +0800, Zhi Yong Wu wrote:
 Note:
   1.) When bps/iops limits are specified to a small value such as 511 
 bytes/s, this VM will hang up. We are considering how to handle this senario.
   2.) When dd command is issued in guest, if its option bs is set to a 
 large value such as bs=1024K, the result speed will slightly bigger than 
 the limits.
 
 For these problems, if you have nice thought, pls let us know.:)
 
 Signed-off-by: Zhi Yong Wu wu...@linux.vnet.ibm.com
 ---
  block.c |  347 
 +--
  block.h |6 +-
  block_int.h |   30 +
  3 files changed, 372 insertions(+), 11 deletions(-)
 
 diff --git a/block.c b/block.c
 index 24a25d5..8fd6643 100644
 --- a/block.c
 +++ b/block.c
 @@ -29,6 +29,9 @@
  #include module.h
  #include qemu-objects.h
 
 +#include qemu-timer.h
 +#include block/blk-queue.h
 +
  #ifdef CONFIG_BSD
  #include sys/types.h
  #include sys/stat.h
 @@ -58,6 +61,13 @@ static int bdrv_read_em(BlockDriverState *bs, int64_t 
 sector_num,
  static int bdrv_write_em(BlockDriverState *bs, int64_t sector_num,
   const uint8_t *buf, int nb_sectors);
 
 +static bool bdrv_exceed_bps_limits(BlockDriverState *bs, int nb_sectors,
 +bool is_write, double elapsed_time, uint64_t *wait);
 +static bool bdrv_exceed_iops_limits(BlockDriverState *bs, bool is_write,
 +double elapsed_time, uint64_t *wait);
 +static bool bdrv_exceed_io_limits(BlockDriverState *bs, int nb_sectors,
 +bool is_write, uint64_t *wait);
 +
  static QTAILQ_HEAD(, BlockDriverState) bdrv_states =
  QTAILQ_HEAD_INITIALIZER(bdrv_states);
 
 @@ -90,6 +100,68 @@ int is_windows_drive(const char *filename)
  }
  #endif
 
 +/* throttling disk I/O limits */
 +void bdrv_io_limits_disable(BlockDriverState *bs)
 +{
 +bs-io_limits_enabled = false;
 +bs-req_from_queue= false;
 +
 +if (bs-block_queue) {
 +qemu_block_queue_flush(bs-block_queue);
 +qemu_del_block_queue(bs-block_queue);
 +}
 +
 +if (bs-block_timer) {
 +qemu_del_timer(bs-block_timer);
 +qemu_free_timer(bs-block_timer);
 +}
 +
 +bs-slice_start[0]   = 0;
 +bs-slice_start[1]   = 0;
 +
 +bs-slice_end[0] = 0;
 +bs-slice_end[1] = 0;
 +}
 +
 +static void bdrv_block_timer(void *opaque)
 +{
 +BlockDriverState *bs = opaque;
 +BlockQueue *queue = bs-block_queue;
 +
 +qemu_block_queue_flush(queue);
 +}
 +
 +void bdrv_io_limits_enable(BlockDriverState *bs)
 +{
 +bs-req_from_queue = false;
 +
 +bs-block_queue= qemu_new_block_queue();
 +bs-block_timer= qemu_new_timer_ns(vm_clock, bdrv_block_timer, bs);
 +
 +bs-slice_start[BLOCK_IO_LIMIT_READ]  = qemu_get_clock_ns(vm_clock);
 +bs-slice_start[BLOCK_IO_LIMIT_WRITE] = qemu_get_clock_ns(vm_clock);
 
a minor comment. better to keep the slice_start of the both the READ and WRITE
side the same.  

bs-slice_start[BLOCK_IO_LIMIT_WRITE] = 
bs-slice_start[BLOCK_IO_LIMIT_READ];

saves  a call to qemu_get_clock_ns().

 +
 +bs-slice_end[BLOCK_IO_LIMIT_READ]=
 +  qemu_get_clock_ns(vm_clock) + BLOCK_IO_SLICE_TIME;

bs-slice_end[BLOCK_IO_LIMIT_READ] = bs-slice_start[BLOCK_IO_LIMIT_READ] +
BLOCK_IO_SLICE_TIME;

saves one more call to qemu_get_clock_ns()

 +bs-slice_end[BLOCK_IO_LIMIT_WRITE]   =
 +  qemu_get_clock_ns(vm_clock) + BLOCK_IO_SLICE_TIME;


bs-slice_end[BLOCK_IO_LIMIT_WRITE] = bs-slice_start[BLOCK_IO_LIMIT_WRITE] 
+
BLOCK_IO_SLICE_TIME;

yet another call saving.


 +}
 +
 +bool bdrv_io_limits_enabled(BlockDriverState *bs)
 +{
 +BlockIOLimit *io_limits = bs-io_limits;
 +if ((io_limits-bps[BLOCK_IO_LIMIT_READ] == 0)
 +  (io_limits-bps[BLOCK_IO_LIMIT_WRITE] == 0)
 +  (io_limits-bps[BLOCK_IO_LIMIT_TOTAL] == 0)
 +  (io_limits-iops[BLOCK_IO_LIMIT_READ] == 0)
 +  (io_limits-iops[BLOCK_IO_LIMIT_WRITE] == 0)
 +  (io_limits-iops[BLOCK_IO_LIMIT_TOTAL] == 0)) {
 +return false;
 +}
 +
 +return true;
 +}

can be optimized to:

return (io_limits-bps[BLOCK_IO_LIMIT_READ]
|| io_limits-bps[BLOCK_IO_LIMIT_WRITE]
|| io_limits-bps[BLOCK_IO_LIMIT_TOTAL]
|| io_limits-iops[BLOCK_IO_LIMIT_READ]
|| io_limits-iops[BLOCK_IO_LIMIT_WRITE]
|| io_limits-iops[BLOCK_IO_LIMIT_TOTAL]);

 +
  /* check if the path starts with protocol: */
  static int path_has_protocol(const char *path)
  {
 @@ -642,6 +714,11 @@ int bdrv_open(BlockDriverState *bs, const char 
 *filename, int flags,
  bs-change_cb(bs-change_opaque, CHANGE_MEDIA);
  }
 
 +/* throttling disk I/O limits */
 +if (bs-io_limits_enabled) {
 +bdrv_io_limits_enable(bs);
 +}
 +
  return 0;
 
  unlink_and_fail:
 @@ -680,6 +757,16 @@ void bdrv_close(BlockDriverState

Re: KVM: VMX: flush TLB with INVEPT on cpu migration

2009-10-02 Thread Ram Pai 

On Thu, 2009-10-01 at 19:16 -0300, Marcelo Tosatti wrote:
 It is possible that stale EPTP-tagged mappings are used, if a 
 vcpu migrates to a different pcpu.
 
 Set KVM_REQ_TLB_FLUSH in vmx_vcpu_load, when switching pcpus, which
 will invalidate both VPID and EPT mappings on the next vm-entry.
 
 Signed-off-by: Marcelo Tosatti mtosa...@redhat.com
 
 diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
 index e86f1a6..97f4265 100644
 --- a/arch/x86/kvm/vmx.c
 +++ b/arch/x86/kvm/vmx.c
 @@ -708,7 +708,7 @@ static void vmx_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
   if (vcpu-cpu != cpu) {
   vcpu_clear(vmx);
   kvm_migrate_timers(vcpu);
 - vpid_sync_vcpu_all(vmx);
 + set_bit(KVM_REQ_TLB_FLUSH, vcpu-requests);
   local_irq_disable();
   list_add(vmx-local_vcpus_link,
per_cpu(vcpus_on_cpu, cpu));
 --

This patch fixes my ept misconfig problem seen very so often while
installing sles11 guest.

thanks,
RP

 

--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] rev7: support colon in filenames

2009-08-06 Thread Ram Pai 
On Tue, 2009-07-21 at 14:42 +0200, Kevin Wolf wrote:
 Ram Pai schrieb:
  Problem: It is impossible to feed filenames with the character colon because
  qemu interprets such names as a protocol. For example filename scsi:0, is
  interpreted as a protocol by name scsi.
  
  This patch allows user to espace colon characters. For example the above
  filename can now be expressed either as 'scsi\:0' or as file:scsi:0
  
  anything following the file: tag is interpreted verbatin. However if 
  file:
  tag is omitted then any colon characters in the string must be escaped using
  backslash.
  
  Here are couple of examples:
  
  scsi\:0\:abc is a local file scsi:0:abc
  http\://myweb is a local file by name http://myweb
  file:scsi:0:abc is a local file scsi:0:abc
  file:http://myweb is a local file by name http://myweb
  
  fat:c:\path\to\dir\:floppy\:  is a fat file by name \path\to\dir:floppy:
  NOTE:The above example cannot be expressed using the file: protocol.
  
  
  Changelog w.r.t to iteration 0:
 1) removes flexibility added to nbd semantics  eg -- nbd:\::
 2) introduce the file: protocol to indicate local file
  
  Changelog w.r.t to iteration 1:
 1) generically handles 'file:' protocol in find_protocol
 2) centralizes 'filename' pruning before the call to open().
 3) fixes buffer overflow seen in fill_token()
 4) adheres to codying style
 5) patch against upstream qemu tree
  
  Changelog w.r.t to iteration 2:
 1) really really fixes buffer overflow seen in 
  fill_token() (if not, beat me :)
 2) the centralized 'filename' pruning had a side effect with
  qcow2 files and other files. Fixed it. _open() is back.
  
  Changelog w.r.t to iteration 3:
 1) support added to raw-win32.c (i do not have the setup to 
  test this change. Request help with testing)
 2) ability to espace option-values containing commas using 
  backslashes 
  eg  file=file:abc,,  can also be expressed as file=file:abc\, 
  where 'abc,' is a filename
 3) fixes a bug (reported by Jan Kiszka) w.r.t support for -snapshot
 4) renamed _open() to qemu_open() and removed dependency on PATH_MAX
  
  Changelog w.r.t to iteration 4:
 1) applies to upstream qemu tree
  
  Changelog w.r.t to iteration 5:
 1) fixed a issue with backing_filename for qcow2 files,
  reported by Jamie Lokier.
 2) fixed a compile issue with win32-raw.c reported by Blue Swirl.
  (I do not have the setup to test win32 changes. 
   Request help with testing)
  
  Changelog w.r.t to iteration 6:
 1) fixed all the issues found with win32. 
  a) changed the call to strnlen() to qemu_strlen() in cutils.c
  b) fixed the call to CreateFile() in qemu_CreateFile()
  
  Signed-off-by: Ram Pai linux...@us.ibm.com
   
  
   block.c   |   38 -
   block/raw-posix.c |   15 
   block/raw-win32.c |   26 --
   block/vvfat.c |   97 
  +++-
   cutils.c  |   46 +
   qemu-common.h |2 +
   qemu-option.c |8 -
   7 files changed, 195 insertions(+), 37 deletions(-)
  
  diff --git a/block.c b/block.c
  index 39f726c..da6eaf7 100644
  --- a/block.c
  +++ b/block.c
  @@ -225,7 +225,6 @@ static BlockDriver *find_protocol(const char *filename)
   {
   BlockDriver *drv1;
   char protocol[128];
  -int len;
   const char *p;
   
   #ifdef _WIN32
  @@ -233,14 +232,9 @@ static BlockDriver *find_protocol(const char *filename)
   is_windows_drive_prefix(filename))
   return bdrv_find_format(raw);
   #endif
  -p = strchr(filename, ':');
  -if (!p)
  +p = prune_strcpy(protocol, sizeof(protocol), filename, ':');
  +if (*p != ':')
   return bdrv_find_format(raw);
  -len = p - filename;
  -if (len  sizeof(protocol) - 1)
  -len = sizeof(protocol) - 1;
  -memcpy(protocol, filename, len);
  -protocol[len] = '\0';
   for(drv1 = first_drv; drv1 != NULL; drv1 = drv1-next) {
   if (drv1-protocol_name 
   !strcmp(drv1-protocol_name, protocol))
  @@ -331,7 +325,6 @@ int bdrv_open2(BlockDriverState *bs, const char 
  *filename, int flags,
   {
   int ret, open_flags;
   char tmp_filename[PATH_MAX];
  -char backing_filename[PATH_MAX];
   
   bs-read_only = 0;
   bs-is_temporary = 0;
  @@ -343,7 +336,6 @@ int bdrv_open2(BlockDriverState *bs, const char 
  *filename, int flags,
   if (flags  BDRV_O_SNAPSHOT) {
   BlockDriverState *bs1;
   int64_t total_size;
  -int is_protocol = 0;
   BlockDriver *bdrv_qcow2;
   QEMUOptionParameter *options;
   
  @@ -359,25 +351,15 @@ int bdrv_open2(BlockDriverState *bs, const char 
  *filename, int flags,
   }
   total_size = bdrv_getlength(bs1)  SECTOR_BITS;
   
  -if (bs1

[PATCH] rev8: support colon in filenames

2009-08-06 Thread Ram Pai 
Problem: It is impossible to feed filenames with the character colon because
qemu interprets such names as a protocol. For example filename scsi:0, is
interpreted as a protocol by name scsi.

This patch allows user to espace colon characters. For example the above
filename can now be expressed either as 'scsi\:0' or as file:scsi:0

anything following the file: tag is interpreted verbatin. However if file:
tag is omitted then any colon characters in the string must be escaped using
backslash.

Here are couple of examples:

scsi\:0\:abc is a local file scsi:0:abc
http\://myweb is a local file by name http://myweb
file:scsi:0:abc is a local file scsi:0:abc
file:http://myweb is a local file by name http://myweb

fat:c:\path\to\dir\:floppy\:  is a fat file by name \path\to\dir:floppy:
NOTE:The above example cannot be expressed using the file: protocol.


Changelog w.r.t to iteration 0:
   1) removes flexibility added to nbd semantics  eg -- nbd:\::
   2) introduce the file: protocol to indicate local file

Changelog w.r.t to iteration 1:
   1) generically handles 'file:' protocol in find_protocol
   2) centralizes 'filename' pruning before the call to open().
   3) fixes buffer overflow seen in fill_token()
   4) adheres to codying style
   5) patch against upstream qemu tree

Changelog w.r.t to iteration 2:
   1) really really fixes buffer overflow seen in 
fill_token() (if not, beat me :)
   2) the centralized 'filename' pruning had a side effect with
qcow2 files and other files. Fixed it. _open() is back.

Changelog w.r.t to iteration 3:
   1) support added to raw-win32.c (i do not have the setup to 
test this change. Request help with testing)
   4) ability to espace option-values containing commas using 
backslashes 
eg  file=file:abc,,  can also be expressed as file=file:abc\, 
where 'abc,' is a filename
   3) fixes a bug (reported by Jan Kiszka) w.r.t support for -snapshot
   4) renamed _open() to qemu_open() and removed dependency on PATH_MAX

Changelog w.r.t to iteration 4:
   1) applies to upstream qemu tree

Changelog w.r.t to iteration 5:
   1) fixed a issue with backing_filename for qcow2 files,
reported by Jamie Lokier.
   2) fixed a compile issue with win32-raw.c reported by Blue Swirl.
(I do not have the setup to test win32 changes. 
 Request help with testing)

Changelog w.r.t to iteration 6:
   1) fixed all the issues found with win32. 
a) changed the call to strnlen() to qemu_strlen() in cutils.c
b) fixed the call to CreateFile() in qemu_CreateFile()

Changelog w.r.t to iteration 7:
   1) fixed buffer overflow issues introduced in get_opt_value() to support
escaping comma using \
   2) added ability in get_opt_value() to express a backslash character using 
a backslash character
   3) moved qemu_open() into raw driver code and renamed the function as 
raw_open2()

Signed-off-by: Ram Pai linux...@us.ibm.com

 block.c   |   38 -
 block/raw-posix.c |   34 +++
 block/raw-win32.c |   26 --
 block/vvfat.c |   97 +++-
 cutils.c  |   26 ++
 qemu-common.h |1 +
 qemu-option.c |6 +++-
 7 files changed, 191 insertions(+), 37 deletions(-)

diff --git a/block.c b/block.c
index 82ffea8..7761dd0 100644
--- a/block.c
+++ b/block.c
@@ -225,7 +225,6 @@ static BlockDriver *find_protocol(const char *filename)
 {
 BlockDriver *drv1;
 char protocol[128];
-int len;
 const char *p;
 
 #ifdef _WIN32
@@ -233,14 +232,9 @@ static BlockDriver *find_protocol(const char *filename)
 is_windows_drive_prefix(filename))
 return bdrv_find_format(raw);
 #endif
-p = strchr(filename, ':');
-if (!p)
+p = prune_strcpy(protocol, sizeof(protocol), filename, ':');
+if (*p != ':')
 return bdrv_find_format(raw);
-len = p - filename;
-if (len  sizeof(protocol) - 1)
-len = sizeof(protocol) - 1;
-memcpy(protocol, filename, len);
-protocol[len] = '\0';
 for(drv1 = first_drv; drv1 != NULL; drv1 = drv1-next) {
 if (drv1-protocol_name 
 !strcmp(drv1-protocol_name, protocol))
@@ -331,7 +325,6 @@ int bdrv_open2(BlockDriverState *bs, const char *filename, 
int flags,
 {
 int ret, open_flags;
 char tmp_filename[PATH_MAX];
-char backing_filename[PATH_MAX];
 
 bs-read_only = 0;
 bs-is_temporary = 0;
@@ -343,7 +336,6 @@ int bdrv_open2(BlockDriverState *bs, const char *filename, 
int flags,
 if (flags  BDRV_O_SNAPSHOT) {
 BlockDriverState *bs1;
 int64_t total_size;
-int is_protocol = 0;
 BlockDriver *bdrv_qcow2;
 QEMUOptionParameter *options;
 
@@ -359,25 +351,15 @@ int bdrv_open2(BlockDriverState *bs, const char 
*filename, int flags,
 }
 total_size = bdrv_getlength(bs1

[PATCH] rev7: support colon in filenames

2009-07-17 Thread Ram Pai 
Problem: It is impossible to feed filenames with the character colon because
qemu interprets such names as a protocol. For example filename scsi:0, is
interpreted as a protocol by name scsi.

This patch allows user to espace colon characters. For example the above
filename can now be expressed either as 'scsi\:0' or as file:scsi:0

anything following the file: tag is interpreted verbatin. However if file:
tag is omitted then any colon characters in the string must be escaped using
backslash.

Here are couple of examples:

scsi\:0\:abc is a local file scsi:0:abc
http\://myweb is a local file by name http://myweb
file:scsi:0:abc is a local file scsi:0:abc
file:http://myweb is a local file by name http://myweb

fat:c:\path\to\dir\:floppy\:  is a fat file by name \path\to\dir:floppy:
NOTE:The above example cannot be expressed using the file: protocol.


Changelog w.r.t to iteration 0:
   1) removes flexibility added to nbd semantics  eg -- nbd:\::
   2) introduce the file: protocol to indicate local file

Changelog w.r.t to iteration 1:
   1) generically handles 'file:' protocol in find_protocol
   2) centralizes 'filename' pruning before the call to open().
   3) fixes buffer overflow seen in fill_token()
   4) adheres to codying style
   5) patch against upstream qemu tree

Changelog w.r.t to iteration 2:
   1) really really fixes buffer overflow seen in 
fill_token() (if not, beat me :)
   2) the centralized 'filename' pruning had a side effect with
qcow2 files and other files. Fixed it. _open() is back.

Changelog w.r.t to iteration 3:
   1) support added to raw-win32.c (i do not have the setup to 
test this change. Request help with testing)
   2) ability to espace option-values containing commas using 
backslashes 
eg  file=file:abc,,  can also be expressed as file=file:abc\, 
where 'abc,' is a filename
   3) fixes a bug (reported by Jan Kiszka) w.r.t support for -snapshot
   4) renamed _open() to qemu_open() and removed dependency on PATH_MAX

Changelog w.r.t to iteration 4:
   1) applies to upstream qemu tree

Changelog w.r.t to iteration 5:
   1) fixed a issue with backing_filename for qcow2 files,
reported by Jamie Lokier.
   2) fixed a compile issue with win32-raw.c reported by Blue Swirl.
(I do not have the setup to test win32 changes. 
 Request help with testing)

Changelog w.r.t to iteration 6:
   1) fixed all the issues found with win32. 
a) changed the call to strnlen() to qemu_strlen() in cutils.c
b) fixed the call to CreateFile() in qemu_CreateFile()

Signed-off-by: Ram Pai linux...@us.ibm.com
 

 block.c   |   38 -
 block/raw-posix.c |   15 
 block/raw-win32.c |   26 --
 block/vvfat.c |   97 +++-
 cutils.c  |   46 +
 qemu-common.h |2 +
 qemu-option.c |8 -
 7 files changed, 195 insertions(+), 37 deletions(-)

diff --git a/block.c b/block.c
index 39f726c..da6eaf7 100644
--- a/block.c
+++ b/block.c
@@ -225,7 +225,6 @@ static BlockDriver *find_protocol(const char *filename)
 {
 BlockDriver *drv1;
 char protocol[128];
-int len;
 const char *p;
 
 #ifdef _WIN32
@@ -233,14 +232,9 @@ static BlockDriver *find_protocol(const char *filename)
 is_windows_drive_prefix(filename))
 return bdrv_find_format(raw);
 #endif
-p = strchr(filename, ':');
-if (!p)
+p = prune_strcpy(protocol, sizeof(protocol), filename, ':');
+if (*p != ':')
 return bdrv_find_format(raw);
-len = p - filename;
-if (len  sizeof(protocol) - 1)
-len = sizeof(protocol) - 1;
-memcpy(protocol, filename, len);
-protocol[len] = '\0';
 for(drv1 = first_drv; drv1 != NULL; drv1 = drv1-next) {
 if (drv1-protocol_name 
 !strcmp(drv1-protocol_name, protocol))
@@ -331,7 +325,6 @@ int bdrv_open2(BlockDriverState *bs, const char *filename, 
int flags,
 {
 int ret, open_flags;
 char tmp_filename[PATH_MAX];
-char backing_filename[PATH_MAX];
 
 bs-read_only = 0;
 bs-is_temporary = 0;
@@ -343,7 +336,6 @@ int bdrv_open2(BlockDriverState *bs, const char *filename, 
int flags,
 if (flags  BDRV_O_SNAPSHOT) {
 BlockDriverState *bs1;
 int64_t total_size;
-int is_protocol = 0;
 BlockDriver *bdrv_qcow2;
 QEMUOptionParameter *options;
 
@@ -359,25 +351,15 @@ int bdrv_open2(BlockDriverState *bs, const char 
*filename, int flags,
 }
 total_size = bdrv_getlength(bs1)  SECTOR_BITS;
 
-if (bs1-drv  bs1-drv-protocol_name)
-is_protocol = 1;
-
 bdrv_delete(bs1);
 
 get_tmp_filename(tmp_filename, sizeof(tmp_filename));
 
-/* Real path is meaningless for protocols */
-if (is_protocol)
-snprintf(backing_filename, sizeof(backing_filename

Re: [Qemu-devel] [PATCH] rev5: support colon in filenames

2009-07-16 Thread Ram Pai 
On Wed, 2009-07-15 at 18:04 +0300, Blue Swirl wrote:
 On 7/15/09, Ram Pai linux...@us.ibm.com wrote:
  Problem: It is impossible to feed filenames with the character colon because
   qemu interprets such names as a protocol. For example filename scsi:0, is
   interpreted as a protocol by name scsi.
 
   --- a/block/raw-posix.c
   +++ b/block/raw-posix.c
   +static int qemu_open(const char *filename, int flags, ...)
 
   --- a/block/raw-win32.c
   +++ b/block/raw-win32.c
   +fd = qemu_open(filename, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY,
 
 I bet this won't compile on win32.

yes. good catch. fix is in the next revision(rev 6). However I do not
have a setup to compile and test changes in win32-raw.c . I will have to
rely on somebody to do the testing. 

RP



--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] rev6: support colon in filenames

2009-07-16 Thread Ram Pai 
Problem: It is impossible to feed filenames with the character colon because
qemu interprets such names as a protocol. For example filename scsi:0, is
interpreted as a protocol by name scsi.

This patch allows user to espace colon characters. For example the above
filename can now be expressed either as 'scsi\:0' or as file:scsi:0

anything following the file: tag is interpreted verbatin. However if file:
tag is omitted then any colon characters in the string must be escaped using
backslash.

Here are couple of examples:

scsi\:0\:abc is a local file scsi:0:abc
http\://myweb is a local file by name http://myweb
file:scsi:0:abc is a local file scsi:0:abc
file:http://myweb is a local file by name http://myweb

fat:c:\path\to\dir\:floppy\:  is a fat file by name \path\to\dir:floppy:
NOTE:The above example cannot be expressed using the file: protocol.


Changelog w.r.t to iteration 0:
   1) removes flexibility added to nbd semantics  eg -- nbd:\::
   2) introduce the file: protocol to indicate local file

Changelog w.r.t to iteration 1:
   1) generically handles 'file:' protocol in find_protocol
   2) centralizes 'filename' pruning before the call to open().
   3) fixes buffer overflow seen in fill_token()
   4) adheres to codying style
   5) patch against upstream qemu tree

Changelog w.r.t to iteration 2:
   1) really really fixes buffer overflow seen in 
fill_token() (if not, beat me :)
   2) the centralized 'filename' pruning had a side effect with
qcow2 files and other files. Fixed it. _open() is back.

Changelog w.r.t to iteration 3:
   1) support added to raw-win32.c (i do not have the setup to 
test this change. Request help with testing)
   2) ability to espace option-values containing commas using 
backslashes 
eg  file=file:abc,,  can also be expressed as file=file:abc\, 
where 'abc,' is a filename
   3) fixes a bug (reported by Jan Kiszka) w.r.t support for -snapshot
   4) renamed _open() to qemu_open() and removed dependency on PATH_MAX

Changelog w.r.t to iteration 4:
   1) applies to upstream qemu tree

Changelog w.r.t to iteration 5:
   1) fixed a issue with backing_filename for qcow2 files,
reported by Jamie Lokier.
   2) fixed a compile issue with win32-raw.c reported by Blue Swirl.
(I do not have the setup to test win32 changes. 
 Request help with testing)

Signed-off-by: Ram Pai linux...@us.ibm.com

 block.c   |   39 
 block/raw-posix.c |   15 
 block/raw-win32.c |   26 --
 block/vvfat.c |   97 -
 cutils.c  |   46 +++
 qemu-common.h |2 +
 qemu-option.c |8 -
 8 files changed, 196 insertions(+), 37 deletions(-)

diff --git a/block.c b/block.c
index cefbe77..4423c72 100644
--- a/block.c
+++ b/block.c
@@ -225,7 +225,6 @@ static BlockDriver *find_protocol(const char *filename)
 {
 BlockDriver *drv1;
 char protocol[128];
-int len;
 const char *p;
 
 #ifdef _WIN32
@@ -233,14 +232,9 @@ static BlockDriver *find_protocol(const char *filename)
 is_windows_drive_prefix(filename))
 return bdrv_find_format(raw);
 #endif
-p = strchr(filename, ':');
-if (!p)
+p = prune_strcpy(protocol, sizeof(protocol), filename, ':');
+if (*p != ':')
 return bdrv_find_format(raw);
-len = p - filename;
-if (len  sizeof(protocol) - 1)
-len = sizeof(protocol) - 1;
-memcpy(protocol, filename, len);
-protocol[len] = '\0';
 for(drv1 = first_drv; drv1 != NULL; drv1 = drv1-next) {
 if (drv1-protocol_name 
 !strcmp(drv1-protocol_name, protocol))
@@ -331,7 +325,6 @@ int bdrv_open2(BlockDriverState *bs, const char *filename, 
int flags,
 {
 int ret, open_flags;
 char tmp_filename[PATH_MAX];
-char backing_filename[PATH_MAX];
 
 bs-read_only = 0;
 bs-is_temporary = 0;
@@ -343,7 +336,6 @@ int bdrv_open2(BlockDriverState *bs, const char *filename, 
int flags,
 if (flags  BDRV_O_SNAPSHOT) {
 BlockDriverState *bs1;
 int64_t total_size;
-int is_protocol = 0;
 BlockDriver *bdrv_qcow2;
 QEMUOptionParameter *options;
 
@@ -359,25 +351,15 @@ int bdrv_open2(BlockDriverState *bs, const char 
*filename, int flags,
 }
 total_size = bdrv_getlength(bs1)  SECTOR_BITS;
 
-if (bs1-drv  bs1-drv-protocol_name)
-is_protocol = 1;
-
 bdrv_delete(bs1);
 
 get_tmp_filename(tmp_filename, sizeof(tmp_filename));
 
-/* Real path is meaningless for protocols */
-if (is_protocol)
-snprintf(backing_filename, sizeof(backing_filename),
- %s, filename);
-else
-realpath(filename, backing_filename);
-
 bdrv_qcow2 = bdrv_find_format(qcow2);
 options = parse_option_parameters

[PATCH] rev5: support colon in filenames

2009-07-15 Thread Ram Pai 
Problem: It is impossible to feed filenames with the character colon because
qemu interprets such names as a protocol. For example filename scsi:0, is
interpreted as a protocol by name scsi.

This patch allows user to espace colon characters. For example the above
filename can now be expressed either as 'scsi\:0' or as file:scsi:0

anything following the file: tag is interpreted verbatin. However if file:
tag is omitted then any colon characters in the string must be escaped using
backslash.

Here are couple of examples:

scsi\:0\:abc is a local file scsi:0:abc
http\://myweb is a local file by name http://myweb
file:scsi:0:abc is a local file scsi:0:abc
file:http://myweb is a local file by name http://myweb

fat:c:\path\to\dir\:floppy\:  is a fat file by name \path\to\dir:floppy:
NOTE:The above example cannot be expressed using the file: protocol.


Changelog w.r.t to iteration 0:
   1) removes flexibility added to nbd semantics  eg -- nbd:\::
   2) introduce the file: protocol to indicate local file

Changelog w.r.t to iteration 1:
   1) generically handles 'file:' protocol in find_protocol
   2) centralizes 'filename' pruning before the call to open().
   3) fixes buffer overflow seen in fill_token()
   4) adheres to codying style
   5) patch against upstream qemu tree

Changelog w.r.t to iteration 2:
   1) really really fixes buffer overflow seen in 
fill_token() (if not, beat me :)
   2) the centralized 'filename' pruning had a side effect with
qcow2 files and other files. Fixed it. _open() is back.

Changelog w.r.t to iteration 3:
   1) support added to raw-win32.c (i do not have the setup to 
test this change. Request help with testing)
   2) ability to espace option-values containing commas using 
backslashes 
eg  file=file:abc,,  can also be expressed as file=file:abc\, 
where 'abc,' is a filename
   3) fixes a bug (reported by Jan Kiszka) w.r.t support for -snapshot
   4) renamed _open() to qemu_open() and removed dependency on PATH_MAX

Changelog w.r.t to iteration 4:
   1) applies to upstream qemu and qemu-kvm tree
   

Signed-off-by: Ram Pai linux...@us.ibm.com


 block.c   |   30 +++-
 block/raw-posix.c |   35 ++
 block/raw-win32.c |   26 --
 block/vvfat.c |   97 -
 cutils.c  |   26 +
 qemu-common.h |1 +
 qemu-option.c |8 -
 8 files changed, 185 insertions(+), 38 deletions(-)

diff --git a/block.c b/block.c
index cefbe77..178edcc 100644
--- a/block.c
+++ b/block.c
@@ -225,7 +225,6 @@ static BlockDriver *find_protocol(const char *filename)
 {
 BlockDriver *drv1;
 char protocol[128];
-int len;
 const char *p;
 
 #ifdef _WIN32
@@ -233,14 +232,9 @@ static BlockDriver *find_protocol(const char *filename)
 is_windows_drive_prefix(filename))
 return bdrv_find_format(raw);
 #endif
-p = strchr(filename, ':');
-if (!p)
+p = prune_strcpy(protocol, sizeof(protocol), filename, ':');
+if (*p != ':')
 return bdrv_find_format(raw);
-len = p - filename;
-if (len  sizeof(protocol) - 1)
-len = sizeof(protocol) - 1;
-memcpy(protocol, filename, len);
-protocol[len] = '\0';
 for(drv1 = first_drv; drv1 != NULL; drv1 = drv1-next) {
 if (drv1-protocol_name 
 !strcmp(drv1-protocol_name, protocol))
@@ -330,8 +324,6 @@ int bdrv_open2(BlockDriverState *bs, const char *filename, 
int flags,
BlockDriver *drv)
 {
 int ret, open_flags;
-char tmp_filename[PATH_MAX];
-char backing_filename[PATH_MAX];
 
 bs-read_only = 0;
 bs-is_temporary = 0;
@@ -343,9 +335,9 @@ int bdrv_open2(BlockDriverState *bs, const char *filename, 
int flags,
 if (flags  BDRV_O_SNAPSHOT) {
 BlockDriverState *bs1;
 int64_t total_size;
-int is_protocol = 0;
 BlockDriver *bdrv_qcow2;
 QEMUOptionParameter *options;
+char tmp_filename[PATH_MAX];
 
 /* if snapshot, we create a temporary backing file and open it
instead of opening 'filename' directly */
@@ -359,25 +351,15 @@ int bdrv_open2(BlockDriverState *bs, const char 
*filename, int flags,
 }
 total_size = bdrv_getlength(bs1)  SECTOR_BITS;
 
-if (bs1-drv  bs1-drv-protocol_name)
-is_protocol = 1;
-
 bdrv_delete(bs1);
 
 get_tmp_filename(tmp_filename, sizeof(tmp_filename));
 
-/* Real path is meaningless for protocols */
-if (is_protocol)
-snprintf(backing_filename, sizeof(backing_filename),
- %s, filename);
-else
-realpath(filename, backing_filename);
-
 bdrv_qcow2 = bdrv_find_format(qcow2);
 options = parse_option_parameters(, bdrv_qcow2-create_options, 
NULL);
 
 set_option_parameter_int(options, BLOCK_OPT_SIZE, total_size

Re: [PATCH] rev5: support colon in filenames

2009-07-15 Thread Ram Pai 
On Wed, 2009-07-15 at 11:30 +0200, Jan Kiszka wrote:
 Ram Pai wrote:
  Problem: It is impossible to feed filenames with the character colon because
  qemu interprets such names as a protocol. For example filename scsi:0, is
  interpreted as a protocol by name scsi.
  
  This patch allows user to espace colon characters. For example the above
  filename can now be expressed either as 'scsi\:0' or as file:scsi:0
  
  anything following the file: tag is interpreted verbatin. However if 
  file:
  tag is omitted then any colon characters in the string must be escaped using
  backslash.
  
  Here are couple of examples:
  
  scsi\:0\:abc is a local file scsi:0:abc
  http\://myweb is a local file by name http://myweb
  file:scsi:0:abc is a local file scsi:0:abc
  file:http://myweb is a local file by name http://myweb
  
  fat:c:\path\to\dir\:floppy\:  is a fat file by name \path\to\dir:floppy:
  NOTE:The above example cannot be expressed using the file: protocol.
  
  
  Changelog w.r.t to iteration 0:
 1) removes flexibility added to nbd semantics  eg -- nbd:\::
 2) introduce the file: protocol to indicate local file
  
  Changelog w.r.t to iteration 1:
 1) generically handles 'file:' protocol in find_protocol
 2) centralizes 'filename' pruning before the call to open().
 3) fixes buffer overflow seen in fill_token()
 4) adheres to codying style
 5) patch against upstream qemu tree
  
  Changelog w.r.t to iteration 2:
 1) really really fixes buffer overflow seen in 
  fill_token() (if not, beat me :)
 2) the centralized 'filename' pruning had a side effect with
  qcow2 files and other files. Fixed it. _open() is back.
  
  Changelog w.r.t to iteration 3:
 1) support added to raw-win32.c (i do not have the setup to 
  test this change. Request help with testing)
 2) ability to espace option-values containing commas using 
  backslashes 
  eg  file=file:abc,,  can also be expressed as file=file:abc\, 
  where 'abc,' is a filename
 3) fixes a bug (reported by Jan Kiszka) w.r.t support for -snapshot
 
 Yep, that's fixed in this version.
 
 4) renamed _open() to qemu_open() and removed dependency on PATH_MAX
  
  Changelog w.r.t to iteration 4:
 1) applies to upstream qemu and qemu-kvm tree
 
  
  Signed-off-by: Ram Pai linux...@us.ibm.com
  
  
   block.c   |   30 +++-
   block/raw-posix.c |   35 ++
   block/raw-win32.c |   26 --
   block/vvfat.c |   97 
  -
   cutils.c  |   26 +
   qemu-common.h |1 +
   qemu-option.c |8 -
   8 files changed, 185 insertions(+), 38 deletions(-)
  
  diff --git a/block.c b/block.c
 
 ...
 
  @@ -359,25 +351,15 @@ int bdrv_open2(BlockDriverState *bs, const char 
  *filename, int flags,
   }
   total_size = bdrv_getlength(bs1)  SECTOR_BITS;
   
  -if (bs1-drv  bs1-drv-protocol_name)
  -is_protocol = 1;
  -
   bdrv_delete(bs1);
   
   get_tmp_filename(tmp_filename, sizeof(tmp_filename));
   
  -/* Real path is meaningless for protocols */
  -if (is_protocol)
  -snprintf(backing_filename, sizeof(backing_filename),
  - %s, filename);
  -else
  -realpath(filename, backing_filename);
  -
 
 This removes realpath without any replacement, right? Did you verify
 that this doesn't break anything, namely snapshots with relative paths
 for the backing image? Please check commit a817d93 and provide a
 reasoning why it's safe to drop it.

I have verified with relative paths and it works.

After analyzing the code, i came to the conclusion that call to
realpath()  adds no real value. 

The logic in bdrv_open2() is something like this

bdrv_open2()
{
   if (snapshot) {
 backup = realpath(filename);
 filename=generate_a_temp_file();
   }
   drv=parse_and_get_bdrv(filename);
   drv-bdrv_open(filename);
   if (backup) {
 bdrv_open2(backup);
   }
}
  
in the above function, the call to realpath() would have been useful had
it changed the current working directory before calling
bdrv_open2(backup). It does not. If in case any function within
drv-bdrv_open change the cwd, then I expect them to restore before
returning.

Also drv-bdrv_open() can anyway handle relative paths. 

Hence I conclude that the call to realpath() adds no value.

Do you see a flaw in this logic? 

RP
 
 Jan
 


--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Qemu-devel] Re: [PATCH] rev5: support colon in filenames

2009-07-15 Thread Ram Pai 
On Wed, 2009-07-15 at 19:20 +0100, Jamie Lokier wrote:
 Ram Pai wrote:
  I have verified with relative paths and it works.
  
  After analyzing the code, i came to the conclusion that call to
  realpath()  adds no real value. 
  
  The logic in bdrv_open2() is something like this
  
  bdrv_open2()
  {
 if (snapshot) {
   backup = realpath(filename);
   filename=generate_a_temp_file();
 }
 drv=parse_and_get_bdrv(filename);
 drv-bdrv_open(filename);
 if (backup) {
   bdrv_open2(backup);
 }
  }

  in the above function, the call to realpath() would have been useful had
  it changed the current working directory before calling
  bdrv_open2(backup). It does not. If in case any function within
  drv-bdrv_open change the cwd, then I expect them to restore before
  returning.
  
  Also drv-bdrv_open() can anyway handle relative paths. 
  
  Hence I conclude that the call to realpath() adds no value.
  
  Do you see a flaw in this logic? 
 
 I don't know about snapshot, but when a qcow2 file contains a relative
 path to it's backing file, QEMU cannot simply open using that relative
 path, because it's relative to the directory containing the qcow2 file,
 not QEMU's current directory.

I have successfully verified qcow2 files. But then I may not be trying
out the exact thing that you are talking about. Can you give me a test 
case that I can verify. I am pretty sure that the patch would work.
However i have not accumulated enough flight time on qemu; so i can be
wrong :(

And one other thing. Let me know if there a test-suite that I can try
for regressions.

RP

 
 (That said, I find it quite annoying when renaming qcow2 files that
 there's no easy way to rename their backing files, and it's even worse
 when moving qcow2 files which refer to backing files in another
 directory, and _especially_ when the qcow2 file contains an absolute
 path to the backing file and you're asked to move it to another system
 which doesn't have those directories.)
 
 -- Jamie
-- 
Ram Pai
System X Device-Driver Enablement Lead
Linux Technology Center
Beaverton OR-97006
503-5783752 t/l 7753752
linux...@us.ibm.com

--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: qcow2 relative paths (was: [PATCH] rev5: support colon in filenames)

2009-07-15 Thread Ram Pai 
On Wed, 2009-07-15 at 22:04 +0100, Jamie Lokier wrote:
 Ram Pai wrote:
  I have successfully verified qcow2 files. But then I may not be trying
  out the exact thing that you are talking about. Can you give me a test 
  case that I can verify.
 
 Commands tried with qemu-0.10.0-1ubuntu1:
 
 $ mkdir unlikely_subdir
 $ cd unlikely_subdir
 $ qemu-img create -f qcow2 backing.img 10
 Formatting 'backing.img', fmt=qcow2, size=10 kB
 $ qemu-img create -f qcow2 -b ../unlikely_subdir/backing.img main.img 10
 Formatting 'main.img', fmt=qcow2, 
 backing_file=../unlikely_subdir/backing.img, size=10 kB
 $ cd ..
 $ qemu-img info unlikely_subdir/main.img 
 image: unlikely_subdir/main.img
 file format: qcow2
 virtual size: 10K (10240 bytes)
 disk size: 16K
 cluster_size: 4096
 highest_alloc: 16384
 backing file: ../unlikely_subdir/backing.img (actual path: 
 unlikely_subdir/../unlikely_subdir/backing.img)
 
 See especially the actual path line.
 
 $ mv unlikely_subdir other_subdir
 $ ls -l other_subdir
 total 32
 -rw-r--r-- 1 jamie jamie 16384 2009-07-15 21:59 backing.img
 -rw-r--r-- 1 jamie jamie 16384 2009-07-15 21:59 main.img
 $ qemu-img info other_subdir/main.img 
 qemu-img: Could not open 'other_subdir/main.img'

Turns out that I did introduce a bug by deleting the call
to path_combine() before calling bdrv_open() on the backing
filename. However the call to realpath() is still not needed.
It feels like a kludge introduced to stop path_combine() from
munging backing_filename. 

I will send out yet another revision with the fix. I just dont' know
what else I will be breaking without having a regression test
harness. :( 


 
 What an unhelpful error message...  There isn't even a way to find out
 the backing file path which the tool is looking for.

Ok. i have introduced a message towards the effect, in the next revision
of the patch.  Hope that will make things a little easier.

I have to go through the all the other mails to understand what has been
proposed, and what I need to incorporate. Looks like a tall order.

For now i will send out revision 6 in the next few hours.

RP


 
  And one other thing. Let me know if there a test-suite that I can try
  for regressions.
 
 Sorry, I don't know anything about any QEMU test suites.
 
 -- Jamie

RP

--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] rev4: support colon in filenames

2009-07-08 Thread Ram Pai 
Problem: It is impossible to feed filenames with the character colon because
qemu interprets such names as a protocol. For example filename scsi:0, is
interpreted as a protocol by name scsi.

This patch allows user to espace colon characters. For example the above
filename can now be expressed either as 'scsi\:0' or as file:scsi:0

anything following the file: tag is interpreted verbatin. However if file:
tag is omitted then any colon characters in the string must be escaped using
backslash.

Here are couple of examples:

scsi\:0\:abc is a local file scsi:0:abc
http\://myweb is a local file by name http://myweb
file:scsi:0:abc is a local file scsi:0:abc
file:http://myweb is a local file by name http://myweb

fat:c:\path\to\dir\:floppy\:  is a fat file by name \path\to\dir:floppy:
NOTE:The above example cannot be expressed using the file: protocol.


Changelog w.r.t to iteration 0:
   1) removes flexibility added to nbd semantics  eg -- nbd:\::
   2) introduce the file: protocol to indicate local file

Changelog w.r.t to iteration 1:
   1) generically handles 'file:' protocol in find_protocol
   2) centralizes 'filename' pruning before the call to open().
   3) fixes buffer overflow seen in fill_token()
   4) adheres to codying style
   5) patch against upstream qemu tree

Changelog w.r.t to iteration 2:
   1) really really fixes buffer overflow seen in 
fill_token() (if not, beat me :)
   2) the centralized 'filename' pruning had a side effect with
qcow2 files and other files. Fixed it. _open() is back.

Changelog w.r.t to iteration 3:
   1) support added to raw-win32.c (i do not have the setup to 
test this change. Request help with testing)
   2) ability to espace option values containing commas using 
backslashes 
eg  file=file:abc,,  can also be expressed as file=file:abc\, 
where 'abc,' is a filename
   3) fixes a bug (reported by Jan Kiszka) w.r.t support for -snapshot
   4) renamed _open() to qemu_open() and removed dependency on PATH_MAX

the patch applies to qemu-kvm tree only. Will port the patch to qemu tree
after successful test results.

Signed-off-by: Ram Pai linux...@us.ibm.com


 block.c   |   30 +++--
 block/raw-posix.c |   35 +++
 block/raw-win32.c |   26 --
 block/vvfat.c |   97 +++-
 cutils.c  |   26 ++
 qemu-common.h |1 +
 qemu-option.c |8 -
 7 files changed, 185 insertions(+), 38 deletions(-)

diff --git a/block.c b/block.c
index aca5a6d..be10bf9 100644
--- a/block.c
+++ b/block.c
@@ -225,7 +225,6 @@ static BlockDriver *find_protocol(const char *filename)
 {
 BlockDriver *drv1;
 char protocol[128];
-int len;
 const char *p;
 
 #ifdef _WIN32
@@ -233,14 +232,9 @@ static BlockDriver *find_protocol(const char *filename)
 is_windows_drive_prefix(filename))
 return bdrv_find_format(raw);
 #endif
-p = strchr(filename, ':');
-if (!p)
+p = prune_strcpy(protocol, sizeof(protocol), filename, ':');
+if (*p != ':')
 return bdrv_find_format(raw);
-len = p - filename;
-if (len  sizeof(protocol) - 1)
-len = sizeof(protocol) - 1;
-memcpy(protocol, filename, len);
-protocol[len] = '\0';
 for(drv1 = first_drv; drv1 != NULL; drv1 = drv1-next) {
 if (drv1-protocol_name 
 !strcmp(drv1-protocol_name, protocol))
@@ -330,8 +324,6 @@ int bdrv_open2(BlockDriverState *bs, const char *filename, 
int flags,
BlockDriver *drv)
 {
 int ret, open_flags;
-char tmp_filename[PATH_MAX];
-char backing_filename[PATH_MAX];
 
 bs-read_only = 0;
 bs-is_temporary = 0;
@@ -343,9 +335,9 @@ int bdrv_open2(BlockDriverState *bs, const char *filename, 
int flags,
 if (flags  BDRV_O_SNAPSHOT) {
 BlockDriverState *bs1;
 int64_t total_size;
-int is_protocol = 0;
 BlockDriver *bdrv_qcow2;
 QEMUOptionParameter *options;
+char tmp_filename[PATH_MAX];
 
 /* if snapshot, we create a temporary backing file and open it
instead of opening 'filename' directly */
@@ -359,25 +351,15 @@ int bdrv_open2(BlockDriverState *bs, const char 
*filename, int flags,
 }
 total_size = bdrv_getlength(bs1)  SECTOR_BITS;
 
-if (bs1-drv  bs1-drv-protocol_name)
-is_protocol = 1;
-
 bdrv_delete(bs1);
 
 get_tmp_filename(tmp_filename, sizeof(tmp_filename));
 
-/* Real path is meaningless for protocols */
-if (is_protocol)
-snprintf(backing_filename, sizeof(backing_filename),
- %s, filename);
-else
-realpath(filename, backing_filename);
-
 bdrv_qcow2 = bdrv_find_format(qcow2);
 options = parse_option_parameters(, bdrv_qcow2-create_options, 
NULL);
 
 set_option_parameter_int(options, BLOCK_OPT_SIZE, total_size

[PATCH] rev3: support colon in filenames

2009-07-01 Thread Ram Pai 
Problem: It is impossible to feed filenames with the character colon because
qemu interprets such names as a protocol. For example filename scsi:0, is
interpreted as a protocol by name scsi.

This patch allows user to escape colon characters. For example the above
filename can now be expressed either as 'scsi\:0' or as file:scsi:0

anything following the file: tag is interpreted verbatim. However if file:
tag is omitted then any colon characters in the string must be escaped using
backslash.

Here are couple of examples:

scsi\:0\:abc is a local file scsi:0:abc
http\://myweb is a local file by name http://myweb
file:scsi:0:abc is a local file scsi:0:abc
file:http://myweb is a local file by name http://myweb

fat:c:\path\to\dir\:floppy\:  is a fat file by name \path\to\dir:floppy:
NOTE:The above example cannot be expressed using the file: protocol.


Changelog w.r.t to iteration 0:
   1) removes flexibility added to nbd semantics  eg -- nbd:\::
   2) introduce the file: protocol to indicate local file

Changelog w.r.t to iteration 1:
   1) generically handles 'file:' protocol in find_protocol
   2) centralizes 'filename' pruning before the call to open().
   3) fixes buffer overflow seen in fill_token()
   4) adheres to coding style
   5) patch against upstream qemu tree

Changelog w.r.t to iteration 2:
   1) really really fixes buffer overflow seen in fill_token()
   2) the centralized 'filename' pruning had a side effect with
qcow2 files and other files. Fixed it. _open() is back.

Signed-off-by: Ram Pai linux...@us.ibm.com

 block.c   |   10 +
 block/raw-posix.c |   15 
 block/vvfat.c |  100 ++--
 cutils.c  |   40 +
 qemu-common.h |2 +
 5 files changed, 148 insertions(+), 19 deletions(-)

diff --git a/block.c b/block.c
index aca5a6d..7ad4dd9 100644
--- a/block.c
+++ b/block.c
@@ -225,7 +225,6 @@ static BlockDriver *find_protocol(const char *filename)
 {
 BlockDriver *drv1;
 char protocol[128];
-int len;
 const char *p;
 
 #ifdef _WIN32
@@ -233,14 +232,9 @@ static BlockDriver *find_protocol(const char *filename)
 is_windows_drive_prefix(filename))
 return bdrv_find_format(raw);
 #endif
-p = strchr(filename, ':');
-if (!p)
+p = prune_strcpy(protocol, 128, filename, ':');
+if (*p != ':')
 return bdrv_find_format(raw);
-len = p - filename;
-if (len  sizeof(protocol) - 1)
-len = sizeof(protocol) - 1;
-memcpy(protocol, filename, len);
-protocol[len] = '\0';
 for(drv1 = first_drv; drv1 != NULL; drv1 = drv1-next) {
 if (drv1-protocol_name 
 !strcmp(drv1-protocol_name, protocol))
diff --git a/block/raw-posix.c b/block/raw-posix.c
index 41bfa37..8a0c0df 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -151,7 +151,7 @@ static int raw_open_common(BlockDriverState *bs, const char 
*filename,
 s-open_flags |= O_DSYNC;
 
 s-fd = -1;
-fd = open(filename, s-open_flags, 0644);
+fd = _open(filename, s-open_flags, 0644);
 if (fd  0) {
 ret = -errno;
 if (ret == -EROFS)
@@ -844,7 +844,7 @@ static int raw_create(const char *filename, 
QEMUOptionParameter *options)
 options++;
 }
 
-fd = open(filename, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY,
+fd = _open(filename, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY,
   0644);
 if (fd  0)
 return -EIO;
@@ -889,6 +889,7 @@ static BlockDriver bdrv_raw = {
 .bdrv_getlength = raw_getlength,
 
 .create_options = raw_create_options,
+.protocol_name = file,
 };
 
 /***/
@@ -985,7 +986,7 @@ static int hdev_open(BlockDriverState *bs, const char 
*filename, int flags)
 if ( bsdPath[ 0 ] != '\0' ) {
 strcat(bsdPath,s0);
 /* some CDs don't have a partition 0 */
-fd = open(bsdPath, O_RDONLY | O_BINARY | O_LARGEFILE);
+fd = _open(bsdPath, O_RDONLY | O_BINARY | O_LARGEFILE);
 if (fd  0) {
 bsdPath[strlen(bsdPath)-1] = '1';
 } else {
@@ -1037,7 +1038,7 @@ static int fd_open(BlockDriverState *bs)
 #endif
 return -EIO;
 }
-s-fd = open(bs-filename, s-open_flags  ~O_NONBLOCK);
+s-fd = _open(bs-filename, s-open_flags  ~O_NONBLOCK);
 if (s-fd  0) {
 s-fd_error_time = qemu_get_clock(rt_clock);
 s-fd_got_error = 1;
@@ -1133,7 +1134,7 @@ static int hdev_create(const char *filename, 
QEMUOptionParameter *options)
 options++;
 }
 
-fd = open(filename, O_WRONLY | O_BINARY);
+fd = _open(filename, O_WRONLY | O_BINARY);
 if (fd  0)
 return -EIO;
 
@@ -1239,7 +1240,7 @@ static int floppy_eject(BlockDriverState *bs, int 
eject_flag)
 close(s-fd);
 s-fd = -1;
 }
-fd = open(bs-filename, s-open_flags | O_NONBLOCK);
+fd = _open(bs-filename

rev1 [PATCH] support colon in filenames

2009-06-26 Thread Ram Pai 
Problem: It is impossible to feed filenames with the character colon because
qemu interprets such names as a protocol. For example a filename scsi:0, is
interpreted as a protocol by name scsi.

This patch allows user to espace colon characters. For example the above
filename can now be expressed either as 'scsi\:0' or as file:scsi:0

anything following the file: tag is interpreted verbatim. However if file:
tag is omitted then any colon characters in the string must be escaped using
backslash.

Here are couple of examples:

scsi\:0\:abc is a local file scsi:0:abc
http\://myweb is a local file by name http://myweb
file:scsi:0:abc is a local file scsi:0:abc
file:http://myweb is a local file by name http://myweb

Changelog w.r.t to iteration 1:
   1) removes flexibility added to nbd semantics  eg -- nbd:\::
   2) introduce the file: protocol to indicate local file
 
NOTE: no code changes are needed to handle commas in a filename. As
always a comma has to be escaped by a preceding comma.


Signed-off-by: Ram Pai linux...@us.ibm.com
---

 block.c   |   16 ++--
 block/raw-posix.c |   30 +++---
 cutils.c  |   26 ++
 qemu-common.h |1 +
 4 files changed, 56 insertions(+), 17 deletions(-)

diff --git a/block.c b/block.c
index aca5a6d..0064e22 100644
--- a/block.c
+++ b/block.c
@@ -225,22 +225,18 @@ static BlockDriver *find_protocol(const char *filename)
 {
 BlockDriver *drv1;
 char protocol[128];
-int len;
-const char *p;
+const char *f;
+int len = strnlen(filename, 128);
 
 #ifdef _WIN32
 if (is_windows_drive(filename) ||
 is_windows_drive_prefix(filename))
 return bdrv_find_format(raw);
 #endif
-p = strchr(filename, ':');
-if (!p)
-return bdrv_find_format(raw);
-len = p - filename;
-if (len  sizeof(protocol) - 1)
-len = sizeof(protocol) - 1;
-memcpy(protocol, filename, len);
-protocol[len] = '\0';
+f = fill_token(protocol, len, filename, ':');
+if (*f != ':' || !strcmp(protocol, file))
+   return bdrv_find_format(raw);
+
 for(drv1 = first_drv; drv1 != NULL; drv1 = drv1-next) {
 if (drv1-protocol_name 
 !strcmp(drv1-protocol_name, protocol))
diff --git a/block/raw-posix.c b/block/raw-posix.c
index 41bfa37..03d6581 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -124,6 +124,22 @@ static int fd_open(BlockDriverState *bs);
 static int cdrom_reopen(BlockDriverState *bs);
 #endif
 
+static int _open(const char *filename, int flags, ...)
+{
+   char myfile[PATH_MAX];
+   const char *f;
+   va_list ap;
+   va_start(ap, flags);
+
+   if (!strstart(filename, file:, f)) {
+   fill_token(myfile, PATH_MAX, filename, '\0');
+   return  open(myfile, flags, ap);
+   } else {
+   return  open(f, flags, ap);
+   }
+}
+
+
 static int raw_open_common(BlockDriverState *bs, const char *filename,
 int flags)
 {
@@ -151,7 +167,7 @@ static int raw_open_common(BlockDriverState *bs, const char 
*filename,
 s-open_flags |= O_DSYNC;
 
 s-fd = -1;
-fd = open(filename, s-open_flags, 0644);
+fd = _open(filename, s-open_flags, 0644);
 if (fd  0) {
 ret = -errno;
 if (ret == -EROFS)
@@ -844,7 +860,7 @@ static int raw_create(const char *filename, 
QEMUOptionParameter *options)
 options++;
 }
 
-fd = open(filename, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY,
+fd = _open(filename, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY,
   0644);
 if (fd  0)
 return -EIO;
@@ -985,7 +1001,7 @@ static int hdev_open(BlockDriverState *bs, const char 
*filename, int flags)
 if ( bsdPath[ 0 ] != '\0' ) {
 strcat(bsdPath,s0);
 /* some CDs don't have a partition 0 */
-fd = open(bsdPath, O_RDONLY | O_BINARY | O_LARGEFILE);
+fd = _open(bsdPath, O_RDONLY | O_BINARY | O_LARGEFILE);
 if (fd  0) {
 bsdPath[strlen(bsdPath)-1] = '1';
 } else {
@@ -1037,7 +1053,7 @@ static int fd_open(BlockDriverState *bs)
 #endif
 return -EIO;
 }
-s-fd = open(bs-filename, s-open_flags  ~O_NONBLOCK);
+s-fd = _open(bs-filename, s-open_flags  ~O_NONBLOCK);
 if (s-fd  0) {
 s-fd_error_time = qemu_get_clock(rt_clock);
 s-fd_got_error = 1;
@@ -1133,7 +1149,7 @@ static int hdev_create(const char *filename, 
QEMUOptionParameter *options)
 options++;
 }
 
-fd = open(filename, O_WRONLY | O_BINARY);
+fd = _open(filename, O_WRONLY | O_BINARY);
 if (fd  0)
 return -EIO;
 
@@ -1239,7 +1255,7 @@ static int floppy_eject(BlockDriverState *bs, int 
eject_flag)
 close(s-fd);
 s-fd = -1;
 }
-fd = open(bs-filename, s-open_flags | O_NONBLOCK);
+fd = _open(bs

rev2 [PATCH] support colon in filenames

2009-06-26 Thread Ram Pai 
Problem: It is impossible to feed filenames with the character colon because
qemu interprets such names as a protocol. For example filename scsi:0, is
interpreted as a protocol by name scsi.

This patch allows user to espace colon characters. For example the above
filename can now be expressed either as 'scsi\:0' or as file:scsi:0

anything following the file: tag is interpreted verbatin. However if file:
tag is omitted then any colon characters in the string must be escaped using
backslash.

Here are couple of examples:

scsi\:0\:abc is a local file scsi:0:abc
http\://myweb is a local file by name http://myweb
file:scsi:0:abc is a local file scsi:0:abc
file:http://myweb is a local file by name http://myweb

Changelog w.r.t to iteration 0:
   1) removes flexibility added to nbd semantics  eg -- nbd:\::
   2) introduce the file: protocol to indicate local file

Changelog w.r.t to iteration 1:
   1) generically handles 'file:' protocol in find_protocol
   2) centralizes 'filename' pruning before the call to open().
   3) fixes buffer overflow seen in fill_token()
   4) adheres to codying style
   5) patch against upstream qemu tree
 
Signed-off-by: Ram Pai linux...@us.ibm.com
---

 block.c   |   27 +--
 block.h   |2 ++
 block/dmg.c   |2 +-
 block/raw-posix.c |1 +
 cutils.c  |   26 ++
 qemu-common.h |1 +
 6 files changed, 48 insertions(+), 11 deletions(-)

diff --git a/block.c b/block.c
index aca5a6d..3fe9317 100644
--- a/block.c
+++ b/block.c
@@ -225,7 +225,7 @@ static BlockDriver *find_protocol(const char *filename)
 {
 BlockDriver *drv1;
 char protocol[128];
-int len;
+int len = strnlen(filename, 127)+1;
 const char *p;
 
 #ifdef _WIN32
@@ -233,14 +233,9 @@ static BlockDriver *find_protocol(const char *filename)
 is_windows_drive_prefix(filename))
 return bdrv_find_format(raw);
 #endif
-p = strchr(filename, ':');
-if (!p)
+p = fill_token(protocol, len, filename, ':');
+if (*p != ':')
 return bdrv_find_format(raw);
-len = p - filename;
-if (len  sizeof(protocol) - 1)
-len = sizeof(protocol) - 1;
-memcpy(protocol, filename, len);
-protocol[len] = '\0';
 for(drv1 = first_drv; drv1 != NULL; drv1 = drv1-next) {
 if (drv1-protocol_name 
 !strcmp(drv1-protocol_name, protocol))
@@ -414,9 +409,9 @@ int bdrv_open2(BlockDriverState *bs, const char *filename, 
int flags,
 open_flags = BDRV_O_RDWR | (flags  BDRV_O_CACHE_MASK);
 else
 open_flags = flags  ~(BDRV_O_FILE | BDRV_O_SNAPSHOT);
-ret = drv-bdrv_open(bs, filename, open_flags);
+ret = bdrv_open3(bs, filename, open_flags, drv);
 if ((ret == -EACCES || ret == -EPERM)  !(flags  BDRV_O_FILE)) {
-ret = drv-bdrv_open(bs, filename, open_flags  ~BDRV_O_RDWR);
+ret = bdrv_open3(bs, filename, open_flags  ~BDRV_O_RDWR, drv);
 bs-read_only = 1;
 }
 if (ret  0) {
@@ -461,6 +456,18 @@ int bdrv_open2(BlockDriverState *bs, const char *filename, 
int flags,
 return 0;
 }
 
+int bdrv_open3(BlockDriverState *bs, const char *filename, int flags, 
BlockDriver *drv)
+{
+char myfile[PATH_MAX];
+const char *f;
+
+if (!strstart(filename, file:, f)) {
+fill_token(myfile, PATH_MAX, filename, '\0');
+return drv-bdrv_open(bs,myfile,flags);
+}
+return drv-bdrv_open(bs,f,flags);
+}
+
 void bdrv_close(BlockDriverState *bs)
 {
 if (bs-drv) {
diff --git a/block.h b/block.h
index 71e87fc..b595772 100644
--- a/block.h
+++ b/block.h
@@ -58,6 +58,8 @@ int bdrv_file_open(BlockDriverState **pbs, const char 
*filename, int flags);
 int bdrv_open(BlockDriverState *bs, const char *filename, int flags);
 int bdrv_open2(BlockDriverState *bs, const char *filename, int flags,
BlockDriver *drv);
+int bdrv_open3(BlockDriverState *bs, const char *filename, int flags,
+   BlockDriver *drv);
 void bdrv_close(BlockDriverState *bs);
 int bdrv_check(BlockDriverState *bs);
 int bdrv_read(BlockDriverState *bs, int64_t sector_num,
diff --git a/block/dmg.c b/block/dmg.c
index 262560f..dd98af4 100644
--- a/block/dmg.c
+++ b/block/dmg.c
@@ -94,7 +94,7 @@ dmg_close:
close(s-fd);
/* open raw instead */
bs-drv=bdrv_find_format(raw);
-   return bs-drv-bdrv_open(bs, filename, flags);
+   return bdrv_open3(bs, filename, flags, bs-drv);
 }
 info_begin=read_off(s-fd);
 if(info_begin==0)
diff --git a/block/raw-posix.c b/block/raw-posix.c
index fa1a394..31b68ff 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -892,6 +892,7 @@ static BlockDriver bdrv_raw = {
 .bdrv_getlength = raw_getlength,
 
 .create_options = raw_create_options,
+.protocol_name = file,
 };
 
 /***/
diff --git a/cutils.c b/cutils.c
index 0623cf7

Re: [PATCH] support colon in filenames

2009-06-25 Thread Ram Pai 
On Thu, 2009-06-25 at 11:14 +0200, Kevin Wolf wrote:
 Ram Pai schrieb:
  Copying the qemu-devel mailing list too.
  
  On Wed, 2009-06-24 at 09:58 -0700, Ram Pai wrote:
  Problem: It is impossible to feed filenames with the character colon 
  because 
  qemu interprets such names as a protocol. For example  a filename scsi:0,
  is interpreted as a protocol by name scsi.
 
  This patch allows users to espace colon characters. For example the above 
  filename
  can now be expressed as 'scsi\:0'
 
  Here are couple of examples:
 
  ndb:\::  is treated as a ndb protocol with a hostname ':' on port 
  scsi\:0\:abc is a local file scsi:0:abc
  http\://myweb is a local file by name http://myweb
  nbd\::localhost:2558  is a protocol by name nbd: 
 
 Is there any use in having a host named : or protocol nbd:? I don't
 think so. 

I do not see the utility either. However if one does find a novel use,
the syntax is expressive enough.

 
 The other examples could be achieved much easier by assigning
 the file: protocol to raw, so we would have:
 file:scsi:0:abc
 file:http://myweb

yes. This is something if implemented; would help. But then its another
mechanism for expression. It has to be a separate patch built on top of
this patch, because you will still need escaping characters like space,
comma, etc



 
 This solution wasn't accepted last time because it wouldn't solve the
 problems with other characters like commas (they need to be escaped as
 double comma on the command line) and that won't be solved by this patch
 either.

This patch does handle commas and any other character as long as it is
escaped using backslashes.

I just checked the man page and it says that commas in the filename can
be escaped by commas :( . Ok i will add that feature to my patch and
resend it. 

Will that be acceptable after that?


Thanks,
RP



 
 Kevin
 

--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] support colon in filenames

2009-06-24 Thread Ram Pai 
Problem: It is impossible to feed filenames with the character colon because 
qemu interprets such names as a protocol. For example  a filename scsi:0,
is interpreted as a protocol by name scsi.

This patch allows users to espace colon characters. For example the above 
filename
can now be expressed as 'scsi\:0'

Here are couple of examples:

ndb:\::  is treated as a ndb protocol with a hostname ':' on port 
scsi\:0\:abc is a local file scsi:0:abc
http\://myweb is a local file by name http://myweb
nbd\::localhost:2558  is a protocol by name nbd: 

Signed-off-by: Ram Pai linux...@us.ibm.com
---


 block.c   |   26 +-
 block/nbd.c   |   19 ++-
 block/raw-posix.c |   24 +---
 cutils.c  |   22 ++
 qemu-common.h |1 +
 vl.c  |3 +--
 6 files changed, 72 insertions(+), 23 deletions(-)

diff --git a/block.c b/block.c
index aca5a6d..80bded9 100644
--- a/block.c
+++ b/block.c
@@ -225,22 +225,30 @@ static BlockDriver *find_protocol(const char *filename)
 {
 BlockDriver *drv1;
 char protocol[128];
-int len;
-const char *p;
+char *p = protocol; 
+const char *f=filename;
+int len = strnlen(filename, 128);
 
 #ifdef _WIN32
 if (is_windows_drive(filename) ||
 is_windows_drive_prefix(filename))
 return bdrv_find_format(raw);
 #endif
-p = strchr(filename, ':');
-if (!p)
+while ( f  filename+len ) {
+   if ( *f == ':' )
+   break;
+   if ( *f == '\\') {
+   f++;
+   if ( *f == '\0')
+   break;
+   }
+   *p++ = *f++;
+}
+*p='\0';
+
+if (*f != ':')
 return bdrv_find_format(raw);
-len = p - filename;
-if (len  sizeof(protocol) - 1)
-len = sizeof(protocol) - 1;
-memcpy(protocol, filename, len);
-protocol[len] = '\0';
+
 for(drv1 = first_drv; drv1 != NULL; drv1 = drv1-next) {
 if (drv1-protocol_name 
 !strcmp(drv1-protocol_name, protocol))
diff --git a/block/nbd.c b/block/nbd.c
index 47d4778..a011cc7 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -64,18 +64,27 @@ static int nbd_open(BlockDriverState *bs, const char* 
filename, int flags)
 
 } else {
 uint16_t port;
-char *p, *r;
+char *q, *p, *r;
 char hostname[128];
 
 pstrcpy(hostname, 128, host);
 
-p = strchr(hostname, ':');
-if (p == NULL)
+   q=p=hostname;
+   while ( p  hostname+128 ) {
+   if (*p == ':')
+   break;
+   if ( *p == '\\' )  {
+   p++;
+   if (*p =='\0') 
+   break;
+   }
+   *q++=*p++;
+   }
+if (*p != ':')
 return -EINVAL;
 
-*p = '\0';
+   *q='\0';
 p++;
-
 port = strtol(p, r, 0);
 if (r == p)
 return -EINVAL;
diff --git a/block/raw-posix.c b/block/raw-posix.c
index 41bfa37..98ede17 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -124,6 +124,16 @@ static int fd_open(BlockDriverState *bs);
 static int cdrom_reopen(BlockDriverState *bs);
 #endif
 
+static int _open(const char *filename, int flags, ...)
+{
+   char myfile[PATH_MAX];
+   va_list ap;
+   va_start(ap, flags);
+   return  open(esc_string(myfile, PATH_MAX, filename),
+   flags, ap);
+}
+
+
 static int raw_open_common(BlockDriverState *bs, const char *filename,
 int flags)
 {
@@ -151,7 +161,7 @@ static int raw_open_common(BlockDriverState *bs, const char 
*filename,
 s-open_flags |= O_DSYNC;
 
 s-fd = -1;
-fd = open(filename, s-open_flags, 0644);
+fd = _open(filename, s-open_flags, 0644);
 if (fd  0) {
 ret = -errno;
 if (ret == -EROFS)
@@ -844,7 +854,7 @@ static int raw_create(const char *filename, 
QEMUOptionParameter *options)
 options++;
 }
 
-fd = open(filename, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY,
+fd = _open(filename, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY,
   0644);
 if (fd  0)
 return -EIO;
@@ -985,7 +995,7 @@ static int hdev_open(BlockDriverState *bs, const char 
*filename, int flags)
 if ( bsdPath[ 0 ] != '\0' ) {
 strcat(bsdPath,s0);
 /* some CDs don't have a partition 0 */
-fd = open(bsdPath, O_RDONLY | O_BINARY | O_LARGEFILE);
+fd = _open(bsdPath, O_RDONLY | O_BINARY | O_LARGEFILE);
 if (fd  0) {
 bsdPath[strlen(bsdPath)-1] = '1';
 } else {
@@ -1037,7 +1047,7 @@ static int fd_open(BlockDriverState *bs)
 #endif
 return -EIO;
 }
-s-fd = open(bs-filename, s-open_flags  ~O_NONBLOCK);
+s-fd = _open(bs-filename, s-open_flags  ~O_NONBLOCK

Re: [PATCH] support colon in filenames

2009-06-24 Thread Ram Pai 
On Wed, 2009-06-24 at 22:38 +0530, Balbir Singh wrote:
 * Ram Pai linux...@us.ibm.com [2009-06-24 09:58:59]:
 
  Problem: It is impossible to feed filenames with the character colon 
  because 
  qemu interprets such names as a protocol. For example  a filename scsi:0,
  is interpreted as a protocol by name scsi.
  
  This patch allows users to espace colon characters. For example the above 
  filename
  can now be expressed as 'scsi\:0'
  
  Here are couple of examples:
  
  ndb:\::  is treated as a ndb protocol with a hostname ':' on port 
  scsi\:0\:abc is a local file scsi:0:abc
  http\://myweb is a local file by name http://myweb
  nbd\::localhost:2558  is a protocol by name nbd: 
  
  Signed-off-by: Ram Pai linux...@us.ibm.com
 
 Are colons useful for filenames? Is there a common use case for this?

Yes. files like
 
/dev/disk/by-path/pci-:0b:00.0-sas-phy0:1-0x5000c5000c14b41d:0-lun0-part3

RP

--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] support colon in filenames

2009-06-24 Thread Ram Pai 
Copying the qemu-devel mailing list too.

On Wed, 2009-06-24 at 09:58 -0700, Ram Pai wrote:
 Problem: It is impossible to feed filenames with the character colon because 
 qemu interprets such names as a protocol. For example  a filename scsi:0,
 is interpreted as a protocol by name scsi.
 
 This patch allows users to espace colon characters. For example the above 
 filename
 can now be expressed as 'scsi\:0'
 
 Here are couple of examples:
 
 ndb:\::  is treated as a ndb protocol with a hostname ':' on port 
 scsi\:0\:abc is a local file scsi:0:abc
 http\://myweb is a local file by name http://myweb
 nbd\::localhost:2558  is a protocol by name nbd: 
 
 Signed-off-by: Ram Pai linux...@us.ibm.com
 ---
 
 
  block.c   |   26 +-
  block/nbd.c   |   19 ++-
  block/raw-posix.c |   24 +---
  cutils.c  |   22 ++
  qemu-common.h |1 +
  vl.c  |3 +--
  6 files changed, 72 insertions(+), 23 deletions(-)
 
 diff --git a/block.c b/block.c
 index aca5a6d..80bded9 100644
 --- a/block.c
 +++ b/block.c
 @@ -225,22 +225,30 @@ static BlockDriver *find_protocol(const char *filename)
  {
  BlockDriver *drv1;
  char protocol[128];
 -int len;
 -const char *p;
 +char *p = protocol; 
 +const char *f=filename;
 +int len = strnlen(filename, 128);
 
  #ifdef _WIN32
  if (is_windows_drive(filename) ||
  is_windows_drive_prefix(filename))
  return bdrv_find_format(raw);
  #endif
 -p = strchr(filename, ':');
 -if (!p)
 +while ( f  filename+len ) {
 + if ( *f == ':' )
 + break;
 + if ( *f == '\\') {
 + f++;
 + if ( *f == '\0')
 + break;
 + }
 + *p++ = *f++;
 +}
 +*p='\0';
 +
 +if (*f != ':')
  return bdrv_find_format(raw);
 -len = p - filename;
 -if (len  sizeof(protocol) - 1)
 -len = sizeof(protocol) - 1;
 -memcpy(protocol, filename, len);
 -protocol[len] = '\0';
 +
  for(drv1 = first_drv; drv1 != NULL; drv1 = drv1-next) {
  if (drv1-protocol_name 
  !strcmp(drv1-protocol_name, protocol))
 diff --git a/block/nbd.c b/block/nbd.c
 index 47d4778..a011cc7 100644
 --- a/block/nbd.c
 +++ b/block/nbd.c
 @@ -64,18 +64,27 @@ static int nbd_open(BlockDriverState *bs, const char* 
 filename, int flags)
 
  } else {
  uint16_t port;
 -char *p, *r;
 +char *q, *p, *r;
  char hostname[128];
 
  pstrcpy(hostname, 128, host);
 
 -p = strchr(hostname, ':');
 -if (p == NULL)
 + q=p=hostname;
 + while ( p  hostname+128 ) {
 + if (*p == ':')
 + break;
 + if ( *p == '\\' )  {
 + p++;
 + if (*p =='\0') 
 + break;
 + }
 + *q++=*p++;
 + }
 +if (*p != ':')
  return -EINVAL;
 
 -*p = '\0';
 + *q='\0';
  p++;
 -
  port = strtol(p, r, 0);
  if (r == p)
  return -EINVAL;
 diff --git a/block/raw-posix.c b/block/raw-posix.c
 index 41bfa37..98ede17 100644
 --- a/block/raw-posix.c
 +++ b/block/raw-posix.c
 @@ -124,6 +124,16 @@ static int fd_open(BlockDriverState *bs);
  static int cdrom_reopen(BlockDriverState *bs);
  #endif
 
 +static int _open(const char *filename, int flags, ...)
 +{
 + char myfile[PATH_MAX];
 + va_list ap;
 + va_start(ap, flags);
 + return  open(esc_string(myfile, PATH_MAX, filename),
 + flags, ap);
 +}
 +
 +
  static int raw_open_common(BlockDriverState *bs, const char *filename,
  int flags)
  {
 @@ -151,7 +161,7 @@ static int raw_open_common(BlockDriverState *bs, const 
 char *filename,
  s-open_flags |= O_DSYNC;
 
  s-fd = -1;
 -fd = open(filename, s-open_flags, 0644);
 +fd = _open(filename, s-open_flags, 0644);
  if (fd  0) {
  ret = -errno;
  if (ret == -EROFS)
 @@ -844,7 +854,7 @@ static int raw_create(const char *filename, 
 QEMUOptionParameter *options)
  options++;
  }
 
 -fd = open(filename, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY,
 +fd = _open(filename, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY,
0644);
  if (fd  0)
  return -EIO;
 @@ -985,7 +995,7 @@ static int hdev_open(BlockDriverState *bs, const char 
 *filename, int flags)
  if ( bsdPath[ 0 ] != '\0' ) {
  strcat(bsdPath,s0);
  /* some CDs don't have a partition 0 */
 -fd = open(bsdPath, O_RDONLY | O_BINARY | O_LARGEFILE);
 +fd = _open(bsdPath, O_RDONLY | O_BINARY | O_LARGEFILE);
  if (fd  0) {
  bsdPath[strlen(bsdPath)-1] = '1';
  } else {
 @@ -1037,7 +1047,7 @@ static int fd_open

Re: kvm: emulation failure

2009-06-22 Thread Ram Pai 
On Mon, 2009-06-22 at 13:12 +0800, Sheng Yang wrote:
 On Saturday 20 June 2009 03:23:40 Ram Pai wrote:
  I see this problem with a x86 sles10 guest running on x86_64 intel host.
  If the guest is reset abruptly and rebooted, some where
  before grub sequence it hangs and the following message is seen in the
  logs
 
  emulation failed (pagetable) rip 7ed5 66 60 ac 20.
 
  I located this instruction sequence in isolinux.bin on the iso ;if that
  is relevant.
 
 
  I did some analysis and find that there is an ept violation, which is
  handled and then the next instruction '66 60' is attempted to decode
  and emulate. But decode fails. kvm continues loops in the kernel
  in __vcpu_run().
 
  the code path is
 
  kvm_run() - __vcpu_run() - vcpu_enter_guest() - kvm_handle_exit() -
  handle_ept_violation() - kvm_mmu_page_fault() - emulate_instruction() -
  x86_decode_insn()
 
 
 Hi Ram
 
 Seems KVM failed to emulate a unknown instruction. 
 
   6660  pushad
 0002  AClodsb
 
 And PUSHAD have not implemented in x86_emulate.c.

Thanks Sheng for your response,

Good. that was the conclusion i had reached reading the code. However
was not sure whether the (a) the code path should have never reached
there or (b) the code must have learnt to emulate pushad.

Sounds like (b) is the case.

 
 But I am a little curious about why this code path was only triggered when 
 reset. Maybe other issue exists.

What do you want me to check? I have seen ept violation code getting
triggered a few number of times at various stages. But the one reported
above is the only case where the instruction being emulated is 66 60. 

one more observation:
  seen only if the /boot partition is reiserfs. I have been unable to
  reproduce this with /boot being ext3.


thanks and let me know,
RP

--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: kvm: emulation failure

2009-06-22 Thread Ram Pai 
On Mon, 2009-06-22 at 11:26 +0300, Avi Kivity wrote:
 On 06/22/2009 09:55 AM, Ram Pai wrote:
  On Mon, 2009-06-22 at 13:12 +0800, Sheng Yang wrote:
 
  On Saturday 20 June 2009 03:23:40 Ram Pai wrote:
   
  I see this problem with a x86 sles10 guest running on x86_64 intel host.
  If the guest is reset abruptly and rebooted, some where
  before grub sequence it hangs and the following message is seen in the
  logs
 
  emulation failed (pagetable) rip 7ed5 66 60 ac 20.
 
  I located this instruction sequence in isolinux.bin on the iso ;if that
  is relevant.
 
 
  I did some analysis and find that there is an ept violation, which is
  handled and then the next instruction '66 60' is attempted to decode
  and emulate. But decode fails. kvm continues loops in the kernel
  in __vcpu_run().
 
  the code path is
 
  kvm_run() -  __vcpu_run() -  vcpu_enter_guest() -  kvm_handle_exit() -
  handle_ept_violation() -  kvm_mmu_page_fault() -  emulate_instruction() 
  -
  x86_decode_insn()
 
 
  Hi Ram
 
  Seems KVM failed to emulate a unknown instruction.
 
    6660  pushad
  0002  AClodsb
 
  And PUSHAD have not implemented in x86_emulate.c.
   
 
  Thanks Sheng for your response,
 
  Good. that was the conclusion i had reached reading the code. However
  was not sure whether the (a) the code path should have never reached
  there or (b) the code must have learnt to emulate pushad.
 
  Sounds like (b) is the case.
 
 
 
 With ept, the only reason to emulate is mmio.  It's very unlikely that 
 the guest is using the pusha instruction for mmio, so the guest is 
 probably confused here.
 
 Current kvm.git will return an error here, and current qemu-kvm.git will 
 stop the guest on error so we can debug.  But the real problem likely 
 started much earlier, I'm not sure we'll get much useful information.
 
 Is this problem reproducible?

I can reproduce this at will. the latest qem-kvm.git hangs looping in
the kernel, spewing out regularly the following message
emulation failed (pagetable) rip 7ed5 66 60 ac 20

the way to reproduce it -- install sles10sp2 i386; I suppose 
one could probably reproduce using a opensuse guest too(though I have
not tried myself). After installation, reset the
guest through the monitor, and the guest will hang just before
grub takes control.  Key is to have the --cdrom iso on the command
line.


 
  But I am a little curious about why this code path was only triggered when
  reset. Maybe other issue exists.
   
 
  What do you want me to check? I have seen ept violation code getting
  triggered a few number of times at various stages. But the one reported
  above is the only case where the instruction being emulated is 66 60.
 
  one more observation:
 seen only if the /boot partition is reiserfs. I have been unable to
 reproduce this with /boot being ext3.
 
 
 Please try it with current sources and post the output of 'info 
 registers' in the monitor.

EAX=0080 EBX= ECX= EDX=
ESI=aa1a EDI=0004cd0c EBP= ESP=d562
EIP=9e67 EFL=00033282 [--S] CPL=3 II=0 A20=1 SMM=0 HLT=0
ES =   f300
CS =   f300
SS =99e9 00099e90  f300
DS =   f300
FS =   f300
GS =   f300
LDT=   8200
TR =0080 fffbd000 2088 8b00
GDT= 00040620 000f
IDT=  03ff
CR0=0010 CR2= CR3= CR4=
DR0= DR1= DR2= DR3= 
DR6=0ff0 DR7=0400
FCW=037f FSW= [ST=0] FTW=00 MXCSR=
FPR0=  FPR1= 
FPR2=  FPR3= 
FPR4=  FPR5= 
FPR6=  FPR7= 
XMM00= XMM01=
XMM02= XMM03=
XMM04= XMM05=
XMM06= XMM07=



RP



--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

kvm: emulation failure

2009-06-19 Thread Ram Pai 
I see this problem with a x86 sles10 guest running on x86_64 intel host.
If the guest is reset abruptly and rebooted, some where 
before grub sequence it hangs and the following message is seen in the
logs

emulation failed (pagetable) rip 7ed5 66 60 ac 20.

I located this instruction sequence in isolinux.bin on the iso ;if that
is relevant.


I did some analysis and find that there is an ept violation, which is
handled and then the next instruction '66 60' is attempted to decode 
and emulate. But decode fails. kvm continues loops in the kernel
in __vcpu_run().

the code path is 

kvm_run() - __vcpu_run() - vcpu_enter_guest() - kvm_handle_exit() -
handle_ept_violation() - kvm_mmu_page_fault() - emulate_instruction() -
x86_decode_insn()


Any insights here on how to fix the problem is appreciated. And if a fix 
already exists even better :)

thanks,
RP

--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html