[Qemu-devel] [PATCH] Make VNC support optional
From: Jes Sorensen jes.soren...@redhat.com
Per default VNC is enabled.
Signed-off-by: Jes Sorensen jes.soren...@redhat.com
---
Makefile.objs | 19 ++-
configure | 37 +
monitor.c | 16
vl.c | 10 +-
4 files changed, 60 insertions(+), 22 deletions(-)
diff --git a/Makefile.objs b/Makefile.objs
index 9e98a66..58388e2 100644
--- a/Makefile.objs
+++ b/Makefile.objs
@@ -127,19 +127,20 @@ common-obj-y += $(addprefix audio/, $(audio-obj-y))
ui-obj-y += keymaps.o
ui-obj-$(CONFIG_SDL) += sdl.o sdl_zoom.o x_keymap.o
ui-obj-$(CONFIG_CURSES) += curses.o
-ui-obj-y += vnc.o d3des.o
-ui-obj-y += vnc-enc-zlib.o vnc-enc-hextile.o
-ui-obj-y += vnc-enc-tight.o vnc-palette.o
-ui-obj-y += vnc-enc-zrle.o
-ui-obj-$(CONFIG_VNC_TLS) += vnc-tls.o vnc-auth-vencrypt.o
-ui-obj-$(CONFIG_VNC_SASL) += vnc-auth-sasl.o
-ui-obj-$(CONFIG_COCOA) += cocoa.o
+vnc-obj-y += vnc.o d3des.o
+vnc-obj-y += vnc-enc-zlib.o vnc-enc-hextile.o
+vnc-obj-y += vnc-enc-tight.o vnc-palette.o
+vnc-obj-y += vnc-enc-zrle.o
+vnc-obj-$(CONFIG_VNC_TLS) += vnc-tls.o vnc-auth-vencrypt.o
+vnc-obj-$(CONFIG_VNC_SASL) += vnc-auth-sasl.o
+vnc-obj-$(CONFIG_COCOA) += cocoa.o
ifdef CONFIG_VNC_THREAD
-ui-obj-y += vnc-jobs-async.o
+vnc-obj-y += vnc-jobs-async.o
else
-ui-obj-y += vnc-jobs-sync.o
+vnc-obj-y += vnc-jobs-sync.o
endif
common-obj-y += $(addprefix ui/, $(ui-obj-y))
+common-obj-$(CONFIG_VNC) += $(addprefix ui/, $(vnc-obj-y))
common-obj-y += iov.o acl.o
common-obj-$(CONFIG_THREAD) += qemu-thread.o
diff --git a/configure b/configure
index 39cdf2b..a64b750 100755
--- a/configure
+++ b/configure
@@ -117,6 +117,7 @@ kvm=
kvm_para=
nptl=
sdl=
+vnc=yes
sparse=no
uuid=
vde=
@@ -539,6 +540,10 @@ for opt do
;;
--enable-sdl) sdl=yes
;;
+ --disable-vnc) vnc=no
+ ;;
+ --enable-vnc) vnc=yes
+ ;;
--fmod-lib=*) fmod_lib=$optarg
;;
--fmod-inc=*) fmod_inc=$optarg
@@ -836,6 +841,8 @@ echo --disable-strip disable stripping binaries
echo --disable-werror disable compilation abort on warning
echo --disable-sdldisable SDL
echo --enable-sdl enable SDL
+echo --disable-vncdisable VNC
+echo --enable-vnc enable VNC
echo --enable-cocoa enable COCOA (Mac OS X only)
echo --audio-drv-list=LISTset audio drivers list:
echoAvailable drivers: $audio_possible_drivers
@@ -1273,7 +1280,7 @@ fi
##
# VNC TLS detection
-if test $vnc_tls != no ; then
+if test $vnc = yes -a $vnc_tls != no ; then
cat $TMPC EOF
#include gnutls/gnutls.h
int main(void) { gnutls_session_t s; gnutls_init(s, GNUTLS_SERVER); return 0;
}
@@ -1293,7 +1300,7 @@ fi
##
# VNC SASL detection
-if test $vnc_sasl != no ; then
+if test $vnc = yes -a $vnc_sasl != no ; then
cat $TMPC EOF
#include sasl/sasl.h
#include stdio.h
@@ -1315,7 +1322,7 @@ fi
##
# VNC JPEG detection
-if test $vnc_jpeg != no ; then
+if test $vnc = yes -a $vnc_jpeg != no ; then
cat $TMPC EOF
#include stdio.h
#include jpeglib.h
@@ -1336,7 +1343,7 @@ fi
##
# VNC PNG detection
-if test $vnc_png != no ; then
+if test $vnc = yes -a $vnc_png != no ; then
cat $TMPC EOF
//#include stdio.h
#include png.h
@@ -2495,11 +2502,14 @@ echo Audio drivers $audio_drv_list
echo Extra audio cards $audio_card_list
echo Block whitelist $block_drv_whitelist
echo Mixer emulation $mixemu
-echo VNC TLS support $vnc_tls
-echo VNC SASL support $vnc_sasl
-echo VNC JPEG support $vnc_jpeg
-echo VNC PNG support $vnc_png
-echo VNC thread$vnc_thread
+echo VNC support $vnc
+if test $vnc = yes ; then
+echo VNC TLS support $vnc_tls
+echo VNC SASL support $vnc_sasl
+echo VNC JPEG support $vnc_jpeg
+echo VNC PNG support $vnc_png
+echo VNC thread$vnc_thread
+fi
if test -n $sparc_cpu; then
echo Target Sparc Arch $sparc_cpu
fi
@@ -2649,6 +2659,9 @@ echo CONFIG_BDRV_WHITELIST=$block_drv_whitelist
$config_host_mak
if test $mixemu = yes ; then
echo CONFIG_MIXEMU=y $config_host_mak
fi
+if test $vnc = yes ; then
+ echo CONFIG_VNC=y $config_host_mak
+fi
if test $vnc_tls = yes ; then
echo CONFIG_VNC_TLS=y $config_host_mak
echo VNC_TLS_CFLAGS=$vnc_tls_cflags $config_host_mak
@@ -2657,15 +2670,15 @@ if test $vnc_sasl = yes ; then
echo CONFIG_VNC_SASL=y $config_host_mak
echo VNC_SASL_CFLAGS=$vnc_sasl_cflags $config_host_mak
fi
-if test $vnc_jpeg != no ; then
+if test $vnc_jpeg = yes ; then
echo CONFIG_VNC_JPEG=y $config_host_mak
echo VNC_JPEG_CFLAGS=$vnc_jpeg_cflags $config_host_mak
fi
-if test $vnc_png != no ; then
+if test $vnc_png = yes ; then
echo CONFIG_VNC_PNG=y $config_host_mak
echo VNC_PNG_CFLAGS=$vnc_png_cflags $config_host_mak
fi
-if test
[Qemu-devel] Re: [PATCH] Make VNC support optional
On 03/11/11 14:36, Anthony Liguori wrote:
diff --git a/monitor.c b/monitor.c
index 22ae3bb..4425315 100644
--- a/monitor.c
+++ b/monitor.c
@@ -441,6 +441,7 @@ void monitor_protocol_event(MonitorEvent event,
QObject *data)
case QEVENT_RESUME:
event_name = RESUME;
break;
+#ifdef CONFIG_VNC
case QEVENT_VNC_CONNECTED:
event_name = VNC_CONNECTED;
break;
@@ -450,6 +451,7 @@ void monitor_protocol_event(MonitorEvent event,
QObject *data)
case QEVENT_VNC_DISCONNECTED:
event_name = VNC_DISCONNECTED;
break;
+#endif
No need to if this out.
I realized that, but I figured it would reduce the code a bit more. I'll
leave it in.
@@ -1073,11 +1077,15 @@ static int do_change(Monitor *mon, const QDict
*qdict, QObject **ret_data)
const char *arg = qdict_get_try_str(qdict, arg);
int ret;
+#ifdef CONFIG_VNC
if (strcmp(device, vnc) == 0) {
ret = do_change_vnc(mon, target, arg);
} else {
+#endif
ret = do_change_block(mon, device, target, arg);
+#ifdef CONFIG_VNC
}
+#endif
return ret;
}
Or this stuff.
Provide a stub function for changing the VNC password and have it return
a failure. Then this function can check that failure and throw a proper
QError indicating that VNC is not present.
Ok, I can do that.
@@ -3002,6 +3014,7 @@ static const mon_cmd_t info_cmds[] = {
.user_print = do_info_mice_print,
.mhandler.info_new = do_info_mice,
},
+#ifdef CONFIG_VNC
{
.name = vnc,
.args_type = ,
@@ -3010,6 +3023,7 @@ static const mon_cmd_t info_cmds[] = {
.user_print = do_info_vnc_print,
.mhandler.info_new = do_info_vnc,
},
+#endif
We don't want to hide commands based on compile settings.
Otherwise, looks good.
I am not sure I follow you here, you prefer to have the commands visible
even though they are disabled? There are other commands which get
disabled like this as well.
Cheers,
Jes
[Qemu-devel] [PATCH v2] Make VNC support optional
From: Jes Sorensen jes.soren...@redhat.com
Per default VNC is enabled.
Signed-off-by: Jes Sorensen jes.soren...@redhat.com
---
Makefile.objs | 19 ++-
configure | 37 +
console.h | 26 --
monitor.c | 22 ++
qerror.h |3 +++
ui/vnc.c | 14 ++
vl.c | 10 +-
7 files changed, 91 insertions(+), 40 deletions(-)
diff --git a/Makefile.objs b/Makefile.objs
index 9e98a66..58388e2 100644
--- a/Makefile.objs
+++ b/Makefile.objs
@@ -127,19 +127,20 @@ common-obj-y += $(addprefix audio/, $(audio-obj-y))
ui-obj-y += keymaps.o
ui-obj-$(CONFIG_SDL) += sdl.o sdl_zoom.o x_keymap.o
ui-obj-$(CONFIG_CURSES) += curses.o
-ui-obj-y += vnc.o d3des.o
-ui-obj-y += vnc-enc-zlib.o vnc-enc-hextile.o
-ui-obj-y += vnc-enc-tight.o vnc-palette.o
-ui-obj-y += vnc-enc-zrle.o
-ui-obj-$(CONFIG_VNC_TLS) += vnc-tls.o vnc-auth-vencrypt.o
-ui-obj-$(CONFIG_VNC_SASL) += vnc-auth-sasl.o
-ui-obj-$(CONFIG_COCOA) += cocoa.o
+vnc-obj-y += vnc.o d3des.o
+vnc-obj-y += vnc-enc-zlib.o vnc-enc-hextile.o
+vnc-obj-y += vnc-enc-tight.o vnc-palette.o
+vnc-obj-y += vnc-enc-zrle.o
+vnc-obj-$(CONFIG_VNC_TLS) += vnc-tls.o vnc-auth-vencrypt.o
+vnc-obj-$(CONFIG_VNC_SASL) += vnc-auth-sasl.o
+vnc-obj-$(CONFIG_COCOA) += cocoa.o
ifdef CONFIG_VNC_THREAD
-ui-obj-y += vnc-jobs-async.o
+vnc-obj-y += vnc-jobs-async.o
else
-ui-obj-y += vnc-jobs-sync.o
+vnc-obj-y += vnc-jobs-sync.o
endif
common-obj-y += $(addprefix ui/, $(ui-obj-y))
+common-obj-$(CONFIG_VNC) += $(addprefix ui/, $(vnc-obj-y))
common-obj-y += iov.o acl.o
common-obj-$(CONFIG_THREAD) += qemu-thread.o
diff --git a/configure b/configure
index 39cdf2b..a64b750 100755
--- a/configure
+++ b/configure
@@ -117,6 +117,7 @@ kvm=
kvm_para=
nptl=
sdl=
+vnc=yes
sparse=no
uuid=
vde=
@@ -539,6 +540,10 @@ for opt do
;;
--enable-sdl) sdl=yes
;;
+ --disable-vnc) vnc=no
+ ;;
+ --enable-vnc) vnc=yes
+ ;;
--fmod-lib=*) fmod_lib=$optarg
;;
--fmod-inc=*) fmod_inc=$optarg
@@ -836,6 +841,8 @@ echo --disable-strip disable stripping binaries
echo --disable-werror disable compilation abort on warning
echo --disable-sdldisable SDL
echo --enable-sdl enable SDL
+echo --disable-vncdisable VNC
+echo --enable-vnc enable VNC
echo --enable-cocoa enable COCOA (Mac OS X only)
echo --audio-drv-list=LISTset audio drivers list:
echoAvailable drivers: $audio_possible_drivers
@@ -1273,7 +1280,7 @@ fi
##
# VNC TLS detection
-if test $vnc_tls != no ; then
+if test $vnc = yes -a $vnc_tls != no ; then
cat $TMPC EOF
#include gnutls/gnutls.h
int main(void) { gnutls_session_t s; gnutls_init(s, GNUTLS_SERVER); return 0;
}
@@ -1293,7 +1300,7 @@ fi
##
# VNC SASL detection
-if test $vnc_sasl != no ; then
+if test $vnc = yes -a $vnc_sasl != no ; then
cat $TMPC EOF
#include sasl/sasl.h
#include stdio.h
@@ -1315,7 +1322,7 @@ fi
##
# VNC JPEG detection
-if test $vnc_jpeg != no ; then
+if test $vnc = yes -a $vnc_jpeg != no ; then
cat $TMPC EOF
#include stdio.h
#include jpeglib.h
@@ -1336,7 +1343,7 @@ fi
##
# VNC PNG detection
-if test $vnc_png != no ; then
+if test $vnc = yes -a $vnc_png != no ; then
cat $TMPC EOF
//#include stdio.h
#include png.h
@@ -2495,11 +2502,14 @@ echo Audio drivers $audio_drv_list
echo Extra audio cards $audio_card_list
echo Block whitelist $block_drv_whitelist
echo Mixer emulation $mixemu
-echo VNC TLS support $vnc_tls
-echo VNC SASL support $vnc_sasl
-echo VNC JPEG support $vnc_jpeg
-echo VNC PNG support $vnc_png
-echo VNC thread$vnc_thread
+echo VNC support $vnc
+if test $vnc = yes ; then
+echo VNC TLS support $vnc_tls
+echo VNC SASL support $vnc_sasl
+echo VNC JPEG support $vnc_jpeg
+echo VNC PNG support $vnc_png
+echo VNC thread$vnc_thread
+fi
if test -n $sparc_cpu; then
echo Target Sparc Arch $sparc_cpu
fi
@@ -2649,6 +2659,9 @@ echo CONFIG_BDRV_WHITELIST=$block_drv_whitelist
$config_host_mak
if test $mixemu = yes ; then
echo CONFIG_MIXEMU=y $config_host_mak
fi
+if test $vnc = yes ; then
+ echo CONFIG_VNC=y $config_host_mak
+fi
if test $vnc_tls = yes ; then
echo CONFIG_VNC_TLS=y $config_host_mak
echo VNC_TLS_CFLAGS=$vnc_tls_cflags $config_host_mak
@@ -2657,15 +2670,15 @@ if test $vnc_sasl = yes ; then
echo CONFIG_VNC_SASL=y $config_host_mak
echo VNC_SASL_CFLAGS=$vnc_sasl_cflags $config_host_mak
fi
-if test $vnc_jpeg != no ; then
+if test $vnc_jpeg = yes ; then
echo CONFIG_VNC_JPEG=y $config_host_mak
echo VNC_JPEG_CFLAGS=$vnc_jpeg_cflags $config_host_mak
fi
-if test $vnc_png != no ; then
+if test $vnc_png = yes
[Qemu-devel] Re: [PATCH v2] Make VNC support optional
On 03/11/11 15:39, Anthony Liguori wrote:
+#ifdef CONFIG_VNC
/* init remote displays */
if (vnc_display) {
vnc_display_init(ds);
@@ -3088,6 +3095,7 @@ int main(int argc, char **argv, char **envp)
printf(VNC server running on `%s'\n,
vnc_display_local_addr(ds));
}
}
+#endif
So what ends up being the default display if VNC and SDL are both
disabled? Have you tested this?
Then you cry :) actually you just don't get video output, a bit like if
you run with -nographic, except it doesn't try to force taking over
stdio. This is intentional btw :)
v3 will be in your mailbox shortly.
Jes
[Qemu-devel] [PATCH v3] Make VNC support optional
From: Jes Sorensen jes.soren...@redhat.com
Per default VNC is enabled.
Signed-off-by: Jes Sorensen jes.soren...@redhat.com
---
Makefile.objs | 19 ++-
configure | 37 +
console.h | 26 --
monitor.c | 22 ++
qerror.h |3 +++
ui/vnc.c | 14 ++
vl.c | 17 ++---
7 files changed, 96 insertions(+), 42 deletions(-)
diff --git a/Makefile.objs b/Makefile.objs
index 9e98a66..58388e2 100644
--- a/Makefile.objs
+++ b/Makefile.objs
@@ -127,19 +127,20 @@ common-obj-y += $(addprefix audio/, $(audio-obj-y))
ui-obj-y += keymaps.o
ui-obj-$(CONFIG_SDL) += sdl.o sdl_zoom.o x_keymap.o
ui-obj-$(CONFIG_CURSES) += curses.o
-ui-obj-y += vnc.o d3des.o
-ui-obj-y += vnc-enc-zlib.o vnc-enc-hextile.o
-ui-obj-y += vnc-enc-tight.o vnc-palette.o
-ui-obj-y += vnc-enc-zrle.o
-ui-obj-$(CONFIG_VNC_TLS) += vnc-tls.o vnc-auth-vencrypt.o
-ui-obj-$(CONFIG_VNC_SASL) += vnc-auth-sasl.o
-ui-obj-$(CONFIG_COCOA) += cocoa.o
+vnc-obj-y += vnc.o d3des.o
+vnc-obj-y += vnc-enc-zlib.o vnc-enc-hextile.o
+vnc-obj-y += vnc-enc-tight.o vnc-palette.o
+vnc-obj-y += vnc-enc-zrle.o
+vnc-obj-$(CONFIG_VNC_TLS) += vnc-tls.o vnc-auth-vencrypt.o
+vnc-obj-$(CONFIG_VNC_SASL) += vnc-auth-sasl.o
+vnc-obj-$(CONFIG_COCOA) += cocoa.o
ifdef CONFIG_VNC_THREAD
-ui-obj-y += vnc-jobs-async.o
+vnc-obj-y += vnc-jobs-async.o
else
-ui-obj-y += vnc-jobs-sync.o
+vnc-obj-y += vnc-jobs-sync.o
endif
common-obj-y += $(addprefix ui/, $(ui-obj-y))
+common-obj-$(CONFIG_VNC) += $(addprefix ui/, $(vnc-obj-y))
common-obj-y += iov.o acl.o
common-obj-$(CONFIG_THREAD) += qemu-thread.o
diff --git a/configure b/configure
index 39cdf2b..a64b750 100755
--- a/configure
+++ b/configure
@@ -117,6 +117,7 @@ kvm=
kvm_para=
nptl=
sdl=
+vnc=yes
sparse=no
uuid=
vde=
@@ -539,6 +540,10 @@ for opt do
;;
--enable-sdl) sdl=yes
;;
+ --disable-vnc) vnc=no
+ ;;
+ --enable-vnc) vnc=yes
+ ;;
--fmod-lib=*) fmod_lib=$optarg
;;
--fmod-inc=*) fmod_inc=$optarg
@@ -836,6 +841,8 @@ echo --disable-strip disable stripping binaries
echo --disable-werror disable compilation abort on warning
echo --disable-sdldisable SDL
echo --enable-sdl enable SDL
+echo --disable-vncdisable VNC
+echo --enable-vnc enable VNC
echo --enable-cocoa enable COCOA (Mac OS X only)
echo --audio-drv-list=LISTset audio drivers list:
echoAvailable drivers: $audio_possible_drivers
@@ -1273,7 +1280,7 @@ fi
##
# VNC TLS detection
-if test $vnc_tls != no ; then
+if test $vnc = yes -a $vnc_tls != no ; then
cat $TMPC EOF
#include gnutls/gnutls.h
int main(void) { gnutls_session_t s; gnutls_init(s, GNUTLS_SERVER); return 0;
}
@@ -1293,7 +1300,7 @@ fi
##
# VNC SASL detection
-if test $vnc_sasl != no ; then
+if test $vnc = yes -a $vnc_sasl != no ; then
cat $TMPC EOF
#include sasl/sasl.h
#include stdio.h
@@ -1315,7 +1322,7 @@ fi
##
# VNC JPEG detection
-if test $vnc_jpeg != no ; then
+if test $vnc = yes -a $vnc_jpeg != no ; then
cat $TMPC EOF
#include stdio.h
#include jpeglib.h
@@ -1336,7 +1343,7 @@ fi
##
# VNC PNG detection
-if test $vnc_png != no ; then
+if test $vnc = yes -a $vnc_png != no ; then
cat $TMPC EOF
//#include stdio.h
#include png.h
@@ -2495,11 +2502,14 @@ echo Audio drivers $audio_drv_list
echo Extra audio cards $audio_card_list
echo Block whitelist $block_drv_whitelist
echo Mixer emulation $mixemu
-echo VNC TLS support $vnc_tls
-echo VNC SASL support $vnc_sasl
-echo VNC JPEG support $vnc_jpeg
-echo VNC PNG support $vnc_png
-echo VNC thread$vnc_thread
+echo VNC support $vnc
+if test $vnc = yes ; then
+echo VNC TLS support $vnc_tls
+echo VNC SASL support $vnc_sasl
+echo VNC JPEG support $vnc_jpeg
+echo VNC PNG support $vnc_png
+echo VNC thread$vnc_thread
+fi
if test -n $sparc_cpu; then
echo Target Sparc Arch $sparc_cpu
fi
@@ -2649,6 +2659,9 @@ echo CONFIG_BDRV_WHITELIST=$block_drv_whitelist
$config_host_mak
if test $mixemu = yes ; then
echo CONFIG_MIXEMU=y $config_host_mak
fi
+if test $vnc = yes ; then
+ echo CONFIG_VNC=y $config_host_mak
+fi
if test $vnc_tls = yes ; then
echo CONFIG_VNC_TLS=y $config_host_mak
echo VNC_TLS_CFLAGS=$vnc_tls_cflags $config_host_mak
@@ -2657,15 +2670,15 @@ if test $vnc_sasl = yes ; then
echo CONFIG_VNC_SASL=y $config_host_mak
echo VNC_SASL_CFLAGS=$vnc_sasl_cflags $config_host_mak
fi
-if test $vnc_jpeg != no ; then
+if test $vnc_jpeg = yes ; then
echo CONFIG_VNC_JPEG=y $config_host_mak
echo VNC_JPEG_CFLAGS=$vnc_jpeg_cflags $config_host_mak
fi
-if test $vnc_png != no ; then
+if test $vnc_png = yes
[Qemu-devel] Re: [PATCH v2] Make VNC support optional
On 03/11/11 15:55, Anthony Liguori wrote:
On 03/11/2011 08:54 AM, Jes Sorensen wrote:
On 03/11/11 15:39, Anthony Liguori wrote:
+#ifdef CONFIG_VNC
/* init remote displays */
if (vnc_display) {
vnc_display_init(ds);
@@ -3088,6 +3095,7 @@ int main(int argc, char **argv, char **envp)
printf(VNC server running on `%s'\n,
vnc_display_local_addr(ds));
}
}
+#endif
So what ends up being the default display if VNC and SDL are both
disabled? Have you tested this?
Then you cry :) actually you just don't get video output, a bit like if
you run with -nographic, except it doesn't try to force taking over
stdio. This is intentional btw :)
Hrm, that doesn't sound very safe to me. Forcing -nographic would be
better IMHO.
-nographic is bad, it forces a takeover of stdio preventing you from
running the monitor on stdio in this case. Not necessarily what you
want. We could do an alternative mode for this, but -nographic is not
what we want in this case.
Cheers,
Jes
Re: [Qemu-devel] Re: [PATCH v2] Make VNC support optional
On 03/11/11 16:50, Anthony Liguori wrote: On 03/11/2011 09:11 AM, Peter Maydell wrote: (Also the -nographic running of serial over stdio doesn't let you kill qemu with ^C, the way -serial stdio does, which is just annoyingly inconsistent...) So if we want to have another mode that has different characteristics, that's fine, but it should be selectable via the command line regardless of how the build is configured. I don't like the idea of a magic mode that is unlocked when other features are disabled because then there's no good way to test such a feature in a full build. Ok I guess it is a question of whether you consider this a magic mode. It really is the same as -vnc with nothing connected to it. I can add a new mode it you like, or we can teach -nographic to behave sanely - I agree 100% with Peter on this one. Let me know what you want to have? Cheers, Jes
[Qemu-devel] [PATCH] Consolidate DisplaySurface allocation in qemu_alloc_display()
From: Jes Sorensen jes.soren...@redhat.com
This removes various code duplication from console.e and sdl.c
Signed-off-by: Jes Sorensen jes.soren...@redhat.com
---
console.c | 45 +
console.h |3 +++
ui/sdl.c | 21 -
3 files changed, 36 insertions(+), 33 deletions(-)
diff --git a/console.c b/console.c
index 57d6eb5..4939a72 100644
--- a/console.c
+++ b/console.c
@@ -1278,35 +1278,40 @@ static DisplaySurface*
defaultallocator_create_displaysurface(int width, int hei
{
DisplaySurface *surface = (DisplaySurface*)
qemu_mallocz(sizeof(DisplaySurface));
-surface-width = width;
-surface-height = height;
-surface-linesize = width * 4;
-surface-pf = qemu_default_pixelformat(32);
-#ifdef HOST_WORDS_BIGENDIAN
-surface-flags = QEMU_ALLOCATED_FLAG | QEMU_BIG_ENDIAN_FLAG;
-#else
-surface-flags = QEMU_ALLOCATED_FLAG;
-#endif
-surface-data = (uint8_t*) qemu_mallocz(surface-linesize *
surface-height);
-
+int linesize = width * 4;
+surface = qemu_alloc_display(surface, width, height, linesize,
+ qemu_default_pixelformat(32), 0);
return surface;
}
static DisplaySurface* defaultallocator_resize_displaysurface(DisplaySurface
*surface,
int width, int height)
{
+int linesize = width * 4;
+surface = qemu_alloc_display(surface, width, height, linesize,
+ qemu_default_pixelformat(32), 0);
+return surface;
+}
+
+DisplaySurface*
+qemu_alloc_display(DisplaySurface *surface, int width, int height,
+ int linesize, PixelFormat pf, int newflags)
+{
+void *data;
surface-width = width;
surface-height = height;
-surface-linesize = width * 4;
-surface-pf = qemu_default_pixelformat(32);
-if (surface-flags QEMU_ALLOCATED_FLAG)
-surface-data = (uint8_t*) qemu_realloc(surface-data,
surface-linesize * surface-height);
-else
-surface-data = (uint8_t*) qemu_malloc(surface-linesize *
surface-height);
+surface-linesize = linesize;
+surface-pf = pf;
+if (surface-flags QEMU_ALLOCATED_FLAG) {
+data = qemu_realloc(surface-data,
+surface-linesize * surface-height);
+} else {
+data = qemu_malloc(surface-linesize * surface-height);
+}
+surface-data = (uint8_t *)data;
+surface-flags = newflags | QEMU_ALLOCATED_FLAG;
#ifdef HOST_WORDS_BIGENDIAN
-surface-flags = QEMU_ALLOCATED_FLAG | QEMU_BIG_ENDIAN_FLAG;
-#else
-surface-flags = QEMU_ALLOCATED_FLAG;
+surface-flags |= QEMU_BIG_ENDIAN_FLAG;
#endif
return surface;
diff --git a/console.h b/console.h
index f4e4741..dec9a76 100644
--- a/console.h
+++ b/console.h
@@ -189,6 +189,9 @@ void register_displaystate(DisplayState *ds);
DisplayState *get_displaystate(void);
DisplaySurface* qemu_create_displaysurface_from(int width, int height, int bpp,
int linesize, uint8_t *data);
+DisplaySurface* qemu_alloc_display(DisplaySurface *surface, int width,
+ int height, int linesize,
+ PixelFormat pf, int newflags);
PixelFormat qemu_different_endianness_pixelformat(int bpp);
PixelFormat qemu_default_pixelformat(int bpp);
diff --git a/ui/sdl.c b/ui/sdl.c
index 47ac49c..6c10ea6 100644
--- a/ui/sdl.c
+++ b/ui/sdl.c
@@ -176,23 +176,18 @@ static DisplaySurface* sdl_create_displaysurface(int
width, int height)
surface-width = width;
surface-height = height;
-
+
if (scaling_active) {
+int linesize;
+PixelFormat pf;
if (host_format.BytesPerPixel != 2 host_format.BytesPerPixel != 4) {
-surface-linesize = width * 4;
-surface-pf = qemu_default_pixelformat(32);
+linesize = width * 4;
+pf = qemu_default_pixelformat(32);
} else {
-surface-linesize = width * host_format.BytesPerPixel;
-surface-pf = sdl_to_qemu_pixelformat(host_format);
+linesize = width * host_format.BytesPerPixel;
+pf = sdl_to_qemu_pixelformat(host_format);
}
-#ifdef HOST_WORDS_BIGENDIAN
-surface-flags = QEMU_ALLOCATED_FLAG | QEMU_BIG_ENDIAN_FLAG;
-#else
-surface-flags = QEMU_ALLOCATED_FLAG;
-#endif
-surface-data = (uint8_t*) qemu_mallocz(surface-linesize *
surface-height);
-
-return surface;
+return qemu_alloc_display(surface, width, height, linesize, pf, 0);
}
if (host_format.BitsPerPixel == 16)
--
1.7.4
Re: [Qemu-devel] Issue with snapshot outside qcow2 disk - qemu 0.14.0
On 03/10/11 22:04, Stefan Hajnoczi wrote: On Thu, Mar 10, 2011 at 7:57 PM, SAURAV LAHIRI saurav_lah...@yahoo.com wrote: The high level use case is that of being able to backup user specified disks of a VM without having to bring down the VM. Excellent, that sounds exactly like Jes is addressing so future QEMU/KVM releases will hopefully have the live snapshot/merge capability. snapshot_blkdev: Regarding this I do have a couple of questions. 1. If the snapshot cannot be merged then it could mean that there are several snapshot files. One readonly for each of the previous snapshots and the last one being the active one, which handles all the current writes. Post backup If do have to restore to a particular snapshot then i would probably have to copy all the files in the chain and maintain the entire chain. But would it not affect read performance if several snapshot files are maintained, particularly if the VM is hosting a database like mysql ? Could you please clarify. If the VM is not running you can use the qemu-img commit command to merge the snapshot back down into the base image. After that you only have one image file again and can restart the VM. Hopefully the deltas are small enough that this process is quick. In the future a live merge command will take care of this and avoid the downtime. Yep, qemu-img convert should be able to copy it into a single image so you can delete the chain. Cheers, Jes
[Qemu-devel] Re: [PATCH] hmp-commands.hx: fix badly merged client_migrate_info command
On 03/11/11 00:21, Anthony Liguori wrote: On 03/09/2011 09:54 AM, jes.soren...@redhat.com wrote: From: Jes Sorensenjes.soren...@redhat.com client_migrate_info was merged badly, It wasn't merged badly, it was implemented badly. The initial description confused me because it sounded like a bad merge conflict resolution but it just was wrong from the start. I wasn't quite sure where the badness happened, it basically looked like a bad cleanup after a git pull. Sorry if I gave the impression that the merge at your end was to blame. Cheers, Jes
[Qemu-devel] Re: [RFC][PATCH v7 05/16] virtagent: common helpers and init routines
On 03/07/11 21:10, Michael Roth wrote: +#define VA_PIDFILE /var/run/qemu-va.pid +#define VA_HDR_LEN_MAX 4096 /* http header limit */ +#define VA_CONTENT_LEN_MAX 2*1024*1024 /* rpc/http send limit */ +#define VA_CLIENT_JOBS_MAX 5 /* max client rpcs we can queue */ +#define VA_SERVER_JOBS_MAX 5 /* max server rpcs we can queue */ +#define VA_SERVER_TIMEOUT_MS 5 * 1000 +#define VA_CLIENT_TIMEOUT_MS 5 * 1000 +#define VA_SENTINEL 0xFF +#define VA_BAUDRATE B38400 /* for isa-serial channels */ + I've been after these before - please put the ones that make sense to tune into a config file, and the same with the pidfile. Cheers, Jes
[Qemu-devel] Re: [RFC][PATCH v7 15/16] virtagent: qemu-va, system-level virtagent guest agent
On 03/07/11 21:10, Michael Roth wrote:
Signed-off-by: Michael Roth mdr...@linux.vnet.ibm.com
---
qemu-va.c | 247
+
1 files changed, 247 insertions(+), 0 deletions(-)
create mode 100644 qemu-va.c
diff --git a/qemu-va.c b/qemu-va.c
new file mode 100644
index 000..a9ff56f
--- /dev/null
+++ b/qemu-va.c
@@ -0,0 +1,247 @@
[snip]
+static void become_daemon(void)
+{
+pid_t pid, sid;
+int pidfd;
+char *pidstr;
+
+pid = fork();
+if (pid 0)
+exit(EXIT_FAILURE);
+if (pid 0) {
+exit(EXIT_SUCCESS);
+}
+
+pidfd = open(VA_PIDFILE, O_CREAT|O_RDWR, S_IRUSR|S_IWUSR);
+if (!pidfd || lockf(pidfd, F_TLOCK, 0))
+errx(EXIT_FAILURE, Cannot lock pid file);
+
+if (ftruncate(pidfd, 0) || lseek(pidfd, 0, SEEK_SET))
+ errx(EXIT_FAILURE, Cannot truncate pid file);
+if (asprintf(pidstr, %d, getpid()) == -1)
+errx(EXIT_FAILURE, Cannot allocate memory);
+if (write(pidfd, pidstr, strlen(pidstr)) != strlen(pidstr))
+errx(EXIT_FAILURE, Failed to write pid file);
+free(pidstr);
Coding style - this needs to be fixed.
+umask(0);
+sid = setsid();
+if (sid 0)
+goto fail;
+if ((chdir(/)) 0)
+goto fail;
and again
Cheers,
Jes
[Qemu-devel] Re: [PATCH v2] Improve error handling in do_snapshot_blkdev()
On 03/07/11 11:01, Kevin Wolf wrote: Am 03.03.2011 14:13, schrieb jes.soren...@redhat.com: @@ -591,6 +592,12 @@ int do_snapshot_blkdev(Monitor *mon, const QDict *qdict, QObject **ret_data) goto out; } +strncpy(old_filename, bs-filename, sizeof(old_filename)); +old_filename[1023] = '\0'; qemu has pstrcpy() from cutils.c for this. I'll change it to use pstrcpy(). -abort(); +qerror_report(QERR_OPEN_FILE_FAILED, filename); +error_printf(do_snapshot_blkdev(): Unable to open newly created + snapshot file: \n); +error_printf( %s. Attempting to revert to original image %s\n, That should probably be a colon in %s: Attempting... Also, is the leading space intentional? The colon is already there prior to the \n on the previous printf line. The space was intentional, but maybe that will just confuse people so I will remove it. I added a colon after image: in the last line instead. Look out for v3. Cheers, Jes
[Qemu-devel] [PATCH v3] Improve error handling in do_snapshot_blkdev()
From: Jes Sorensen jes.soren...@redhat.com
In case we cannot open the newly created snapshot image, try to fall
back to the original image file and continue running on that, which
should prevent the guest from aborting.
This is a corner case which can happen if the admin by mistake
specifies the snapshot file on a virtual file system which does not
support O_DIRECT. bdrv_create() does not use O_DIRECT, but the
following open in bdrv_open() does and will then fail.
Signed-off-by: Jes Sorensen jes.soren...@redhat.com
---
blockdev.c | 29 +++--
1 files changed, 23 insertions(+), 6 deletions(-)
diff --git a/blockdev.c b/blockdev.c
index 0690cc8..d52eef0 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -574,9 +574,10 @@ int do_snapshot_blkdev(Monitor *mon, const QDict *qdict,
QObject **ret_data)
const char *filename = qdict_get_try_str(qdict, snapshot_file);
const char *format = qdict_get_try_str(qdict, format);
BlockDriverState *bs;
-BlockDriver *drv, *proto_drv;
+BlockDriver *drv, *old_drv, *proto_drv;
int ret = 0;
int flags;
+char old_filename[1024];
if (!filename) {
qerror_report(QERR_MISSING_PARAMETER, snapshot_file);
@@ -591,6 +592,11 @@ int do_snapshot_blkdev(Monitor *mon, const QDict *qdict,
QObject **ret_data)
goto out;
}
+pstrcpy(old_filename, sizeof(old_filename), bs-filename);
+
+old_drv = bs-drv;
+flags = bs-open_flags;
+
if (!format) {
format = qcow2;
}
@@ -610,7 +616,7 @@ int do_snapshot_blkdev(Monitor *mon, const QDict *qdict,
QObject **ret_data)
}
ret = bdrv_img_create(filename, format, bs-filename,
- bs-drv-format_name, NULL, -1, bs-open_flags);
+ bs-drv-format_name, NULL, -1, flags);
if (ret) {
goto out;
}
@@ -618,15 +624,26 @@ int do_snapshot_blkdev(Monitor *mon, const QDict *qdict,
QObject **ret_data)
qemu_aio_flush();
bdrv_flush(bs);
-flags = bs-open_flags;
bdrv_close(bs);
ret = bdrv_open(bs, filename, flags, drv);
/*
- * If reopening the image file we just created fails, we really
- * are in trouble :(
+ * If reopening the image file we just created fails, fall back
+ * and try to re-open the original image. If that fails too, we
+ * are in serious trouble.
*/
if (ret != 0) {
-abort();
+qerror_report(QERR_OPEN_FILE_FAILED, filename);
+error_printf(do_snapshot_blkdev(): Unable to open newly created
+ snapshot file: \n);
+error_printf(%s. Attempting to revert to original image: %s\n,
+ filename, old_filename);
+ret = bdrv_open(bs, old_filename, flags, old_drv);
+if (ret != 0) {
+error_printf(do_snapshot_blkdev(): Unable to re-open
+ original image - aborting!\n);
+qerror_report(QERR_OPEN_FILE_FAILED, old_filename);
+abort();
+}
}
out:
if (ret) {
--
1.7.4
Re: [Qemu-devel] [PATCH v3] Improve error handling in do_snapshot_blkdev()
On 03/07/11 17:34, Anthony Liguori wrote:
On 03/07/2011 09:27 AM, jes.soren...@redhat.com wrote:
if (ret != 0) {
-abort();
+qerror_report(QERR_OPEN_FILE_FAILED, filename);
+error_printf(do_snapshot_blkdev(): Unable to open newly
created
+ snapshot file: \n);
+error_printf(%s. Attempting to revert to original image: %s\n,
+ filename, old_filename);
You can't combine qerror_report with continued action. qerror_report()
should be a terminal action. You also shouldn't combine error_printf()
with qerror_report().
You should restore the original image file before doing qerror_report()
and just drop the error_printf()s as it's all redundant information.
I would hardly consider it redundant information that it failed and we
try to go back to the original image. That is an error in itself, even
though rolling back is better than abort()ing.
If qerror_report() is a fatal situation, that is problematic. Then we
need qerror_warn() or something as well, which can return non fatal
information.
The printfs are very valuable for the human monitor, but it isn't really
clear to me what is the ideal return value.
Cheers,
Jes
[Qemu-devel] [PATCH] Improve error handling in do_snapshot_blkdev()
From: Jes Sorensen jes.soren...@redhat.com
In case we cannot open the newly created snapshot image, try to fall
back to the original image file and continue running on that, which
should prevent the guest from aborting.
This is a corner case which can happen if the admin by mistake
specifies the snapshot file on a virtual file system which does not
support O_DIRECT. bdrv_create() does not use O_DIRECT, but the
following open in bdrv_open() does and will then fail.
Signed-off-by: Jes Sorensen jes.soren...@redhat.com
---
blockdev.c | 30 --
1 files changed, 24 insertions(+), 6 deletions(-)
diff --git a/blockdev.c b/blockdev.c
index 0690cc8..d43df5e 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -574,9 +574,12 @@ int do_snapshot_blkdev(Monitor *mon, const QDict *qdict,
QObject **ret_data)
const char *filename = qdict_get_try_str(qdict, snapshot_file);
const char *format = qdict_get_try_str(qdict, format);
BlockDriverState *bs;
-BlockDriver *drv, *proto_drv;
+BlockDriver *drv, *old_drv, *proto_drv;
int ret = 0;
int flags;
+char old_filename[1024];
+
+old_filename[1023] = '\0';
if (!filename) {
qerror_report(QERR_MISSING_PARAMETER, snapshot_file);
@@ -591,6 +594,10 @@ int do_snapshot_blkdev(Monitor *mon, const QDict *qdict,
QObject **ret_data)
goto out;
}
+strncpy(old_filename, bs-filename, 1024);
+old_drv = bs-drv;
+flags = bs-open_flags;
+
if (!format) {
format = qcow2;
}
@@ -610,7 +617,7 @@ int do_snapshot_blkdev(Monitor *mon, const QDict *qdict,
QObject **ret_data)
}
ret = bdrv_img_create(filename, format, bs-filename,
- bs-drv-format_name, NULL, -1, bs-open_flags);
+ bs-drv-format_name, NULL, -1, flags);
if (ret) {
goto out;
}
@@ -618,15 +625,26 @@ int do_snapshot_blkdev(Monitor *mon, const QDict *qdict,
QObject **ret_data)
qemu_aio_flush();
bdrv_flush(bs);
-flags = bs-open_flags;
bdrv_close(bs);
ret = bdrv_open(bs, filename, flags, drv);
/*
- * If reopening the image file we just created fails, we really
- * are in trouble :(
+ * If reopening the image file we just created fails, fall back
+ * and try to re-open the original image. If that fails too, we
+ * are in serious trouble.
*/
if (ret != 0) {
-abort();
+qerror_report(QERR_OPEN_FILE_FAILED, filename);
+error_printf(do_snapshot_blkdev(): Unable to open newly created
+ snapshot file: \n);
+error_printf( %s. Attempting to revert to original image %s\n,
+ filename, old_filename);
+ret = bdrv_open(bs, old_filename, flags, old_drv);
+if (ret != 0) {
+error_printf(do_snapshot_blkdev(): Unable to re-open
+ original image - aborting!\n);
+qerror_report(QERR_OPEN_FILE_FAILED, old_filename);
+abort();
+}
}
out:
if (ret) {
--
1.7.4
Re: [Qemu-devel] QEMU: Discussion of separating core functionality vs supportive features
On 03/02/11 14:49, Michael Roth wrote: On 03/02/2011 07:18 AM, Jes Sorensen wrote: I think we need two types for sure, even for the video case, we will still need a control channel as well. However, I don't think it is desirable to split things up more than we have to, so if we can keep it within one client process that is good. Maybe there are cases where it makes sense to split it into more processes, I could be convinced, but I think we really need to be careful making it too much of a complex mess either. Yup, if it's doable I'd prefer a single client process as well. Just hard to predict how difficult it'll be to support 2 or more mechanisms. Although, I'd imagine we'd end up with something like qemu's io loop, with event-driven shmem and fd-based work, which does seem doable. That is pretty much what I had in mind. Will have to see how it works out, but I think it is very feasible :) Cheers, Jes
[Qemu-devel] [PATCH v2] Improve error handling in do_snapshot_blkdev()
From: Jes Sorensen jes.soren...@redhat.com
In case we cannot open the newly created snapshot image, try to fall
back to the original image file and continue running on that, which
should prevent the guest from aborting.
This is a corner case which can happen if the admin by mistake
specifies the snapshot file on a virtual file system which does not
support O_DIRECT. bdrv_create() does not use O_DIRECT, but the
following open in bdrv_open() does and will then fail.
Signed-off-by: Jes Sorensen jes.soren...@redhat.com
---
blockdev.c | 30 --
1 files changed, 24 insertions(+), 6 deletions(-)
diff --git a/blockdev.c b/blockdev.c
index 0690cc8..f52fe8f 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -574,9 +574,10 @@ int do_snapshot_blkdev(Monitor *mon, const QDict *qdict,
QObject **ret_data)
const char *filename = qdict_get_try_str(qdict, snapshot_file);
const char *format = qdict_get_try_str(qdict, format);
BlockDriverState *bs;
-BlockDriver *drv, *proto_drv;
+BlockDriver *drv, *old_drv, *proto_drv;
int ret = 0;
int flags;
+char old_filename[1024];
if (!filename) {
qerror_report(QERR_MISSING_PARAMETER, snapshot_file);
@@ -591,6 +592,12 @@ int do_snapshot_blkdev(Monitor *mon, const QDict *qdict,
QObject **ret_data)
goto out;
}
+strncpy(old_filename, bs-filename, sizeof(old_filename));
+old_filename[1023] = '\0';
+
+old_drv = bs-drv;
+flags = bs-open_flags;
+
if (!format) {
format = qcow2;
}
@@ -610,7 +617,7 @@ int do_snapshot_blkdev(Monitor *mon, const QDict *qdict,
QObject **ret_data)
}
ret = bdrv_img_create(filename, format, bs-filename,
- bs-drv-format_name, NULL, -1, bs-open_flags);
+ bs-drv-format_name, NULL, -1, flags);
if (ret) {
goto out;
}
@@ -618,15 +625,26 @@ int do_snapshot_blkdev(Monitor *mon, const QDict *qdict,
QObject **ret_data)
qemu_aio_flush();
bdrv_flush(bs);
-flags = bs-open_flags;
bdrv_close(bs);
ret = bdrv_open(bs, filename, flags, drv);
/*
- * If reopening the image file we just created fails, we really
- * are in trouble :(
+ * If reopening the image file we just created fails, fall back
+ * and try to re-open the original image. If that fails too, we
+ * are in serious trouble.
*/
if (ret != 0) {
-abort();
+qerror_report(QERR_OPEN_FILE_FAILED, filename);
+error_printf(do_snapshot_blkdev(): Unable to open newly created
+ snapshot file: \n);
+error_printf( %s. Attempting to revert to original image %s\n,
+ filename, old_filename);
+ret = bdrv_open(bs, old_filename, flags, old_drv);
+if (ret != 0) {
+error_printf(do_snapshot_blkdev(): Unable to re-open
+ original image - aborting!\n);
+qerror_report(QERR_OPEN_FILE_FAILED, old_filename);
+abort();
+}
}
out:
if (ret) {
--
1.7.4
Re: [Qemu-devel] [PATCH] Improve error handling in do_snapshot_blkdev()
On 03/03/11 14:06, Stefan Hajnoczi wrote:
On Thu, Mar 3, 2011 at 10:44 AM, jes.soren...@redhat.com wrote:
+char old_filename[1024];
+
+old_filename[1023] = '\0';
if (!filename) {
qerror_report(QERR_MISSING_PARAMETER, snapshot_file);
@@ -591,6 +594,10 @@ int do_snapshot_blkdev(Monitor *mon, const QDict
*qdict, QObject **ret_data)
goto out;
}
+strncpy(old_filename, bs-filename, 1024);
strncpy does not NUL-terminate if you reach the maximum length. The
source buffer is 1024 chars so we should be fine unless there is a bug
somewhere else too, but please move the old_filename[1023] = '\0'
after the strncpy and use sizeof(old_filename) as the maximum instead
of 1024.
Good point, I was trying to catch it but got it backwards :(
Cheers,
Jes
[Qemu-devel] REPOST: [PATCH v3] tracetool: Add optional argument to specify dtrace probe names
From: Jes Sorensen jes.soren...@redhat.com
Optional feature allowing a user to generate the probe list to match
the name of the binary, in case they wish to install qemu under a
different name than qemu-{system,user},arch
Signed-off-by: Jes Sorensen jes.soren...@redhat.com
---
scripts/tracetool | 19 +--
1 files changed, 13 insertions(+), 6 deletions(-)
diff --git a/scripts/tracetool b/scripts/tracetool
index e046683..412f695 100755
--- a/scripts/tracetool
+++ b/scripts/tracetool
@@ -30,9 +30,11 @@ Output formats:
--stap Generate .stp file (DTrace with SystemTAP only)
Options:
- --binary [path] Full path to QEMU binary
- --target-arch [arch] QEMU emulator target arch
- --target-type [type] QEMU emulator target type ('system' or 'user')
+ --binary [path]Full path to QEMU binary
+ --target-arch [arch]QEMU emulator target arch
+ --target-type [type]QEMU emulator target type ('system' or 'user')
+ --probe-prefix [prefix] Prefix for dtrace probe names
+ (default: qemu-\$targettype-\$targetarch)
EOF
exit 1
@@ -472,7 +474,7 @@ linetostap_dtrace()
# Define prototype for probe arguments
cat EOF
-probe qemu.$targettype.$targetarch.$name = process($binary).mark($name)
+probe $probeprefix.$name = process($binary).mark($name)
{
EOF
@@ -574,14 +576,17 @@ tracetostap()
echo --binary is required for SystemTAP tapset generator
exit 1
fi
-if [ -z $targettype ]; then
+if [ -z $probeprefix -a -z $targettype ]; then
echo --target-type is required for SystemTAP tapset generator
exit 1
fi
-if [ -z $targetarch ]; then
+if [ -z $probeprefix -a -z $targetarch ]; then
echo --target-arch is required for SystemTAP tapset generator
exit 1
fi
+if [ -z $probeprefix ]; then
+ probeprefix=qemu.$targettype.$targetarch;
+fi
echo /* This file is autogenerated by tracetool, do not edit. */
convert stap
}
@@ -592,6 +597,7 @@ output=
binary=
targettype=
targetarch=
+probeprefix=
until [ -z $1 ]
@@ -602,6 +608,7 @@ do
--binary) shift ; binary=$1 ;;
--target-arch) shift ; targetarch=$1 ;;
--target-type) shift ; targettype=$1 ;;
+--probe-prefix) shift ; probeprefix=$1 ;;
-h | -c | -d) output=${1#-} ;;
--stap) output=${1#--} ;;
--
1.7.4
Re: [Qemu-devel] QEMU: Discussion of separating core functionality vs supportive features
On 02/28/11 18:44, Anthony Liguori wrote: On Feb 28, 2011 10:44 AM, Jes Sorensen jes.soren...@redhat.com wrote: Separating host-side virtagent and other tasks from core QEMU = To improve auditing of the core QEMU code, it would be ideal to be able to separate the core QEMU functionality from utility functionality by moving the utility functionality into its own process. This process will be referred to as the QEMU client below. Separating as in moving code outside of qemu.git, making code optionally built in, making code optionally built in or loadable as a separate executable that is automatically launched, or making code always built outside the main executable? I'm very nervous about having a large number of daemons necessary to run QEMU. I think a reasonable approach would be a single front-end daemond. Once QAPI is merged, there is a very incremental approach we can take for this sort of work. Take your favorite subsystem (like gdbstub or SDL) and make it only use QMP apis. Once we're only using QMP internally in a subsystem, then building it out of the main QEMU and using libqmp should be fairly easy. Hi Anthony, Back from a day off playing with power drills and concrete walls :) Sorry I should have made it a little more clear, it was obvious to me, but not written down: The idea is to keep everything as part of the QEMU package, ie. part of qemu.git. My idea is really to have one QEMU host daemon and one QEMU client, which provides the various services. You type make and you get two binaries instead of one. We could allow other daemons to connect to the host daemon, but that wouldn't be a primary target for this in my book, and I am not sure we really want to do this. It is absolutely vital for me that we do not make things much more complicated for users with this move. I don't want to get into a situation where we start forcing external packages or daemons in order to run basic QEMU. If we start requiring such, we have failed! However, I think it is a reasonable compromise to have one daemon you launch, and then a simple client you launch straight after which will then provide the same/similar views and interfaces that users are used to when they launch current QEMU (monitor, vnc server etc). I hope this clarifies things. Cheers, Jes
Re: [Qemu-devel] QEMU: Discussion of separating core functionality vs supportive features
On 03/01/11 13:07, Dor Laor wrote: On 02/28/2011 07:44 PM, Anthony Liguori wrote: I'm very nervous about having a large number of daemons necessary to run QEMU. I think a reasonable approach would be a single front-end daemond. s/daemon/son processes/ Qemu is the one that should spawn them and they should be transparent from the management. This way running qemu stays the same and qemu just need to add the logic to get a SIGCHILD and potentially re-execute an dead son process. I disagree here, I do not want to require QEMU to spawn the new processes. Having a daemon you can use to connect will provide more flexibility and isn't unreasonably complex. Cheers, Jes
Re: [Qemu-devel] QEMU: Discussion of separating core functionality vs supportive features
On 03/01/11 15:25, Dor Laor wrote: On 03/01/2011 02:40 PM, Anthony Liguori wrote: On Mar 1, 2011 7:07 AM, Dor Laor dl...@redhat.com Qemu is the one that should spawn them and they should be transparent from the management. This way running qemu stays the same and qemu just need to add the logic to get a SIGCHILD and potentially re-execute an dead son process. Spice is the logical place to start, no? It's the largest single dependency we have and it does some scary things with qemu_mutex. I would use spice as a way to prove the concept. I agree it is desirable to the this for spice but it is allot more complex than virtagent isolation. Spice is performance sensitive and contains much more state. It needs to access the guest memory for reading the surfaces. It can be solved but needs some major changes. Adding spice-devel to the discussion. I had a few thoughts about this already, which I think will work for both spice and vnc. What we could do is to expose the video memory via shared memory. That way a spice or vnc daemon could get direct access to the memory, this would limit communication to keyboard/mouse events, as well as video mode info, and possibly notifications to the client about which ranges of memory have been updated. Using shared memory this way should allow us to implement the video clients without performance loss, in fact it should be beneficial since it would allow them to run fully separate from the host daemon. Cheers, Jes
Re: [Qemu-devel] QEMU: Discussion of separating core functionality vs supportive features
On 03/01/11 15:25, Dor Laor wrote: On 03/01/2011 02:40 PM, Anthony Liguori wrote: Spice is the logical place to start, no? It's the largest single dependency we have and it does some scary things with qemu_mutex. I would use spice as a way to prove the concept. I agree it is desirable to the this for spice but it is allot more complex than virtagent isolation. Spice is performance sensitive and contains much more state. It needs to access the guest memory for reading the surfaces. It can be solved but needs some major changes. Adding spice-devel to the discussion. Please don't add broken misconfigured mailing lists, which require moderator or subscription, to discussions on public lists. Jes
Re: [Qemu-devel] QEMU: Discussion of separating core functionality vs supportive features
On 03/02/11 11:42, Dor Laor wrote: On 03/02/2011 12:28 PM, Jes Sorensen wrote: On 03/01/11 15:25, Dor Laor wrote: I agree it is desirable to the this for spice but it is allot more complex than virtagent isolation. Spice is performance sensitive and contains much more state. It needs to access the guest memory for reading the surfaces. It can be solved but needs some major changes. Adding spice-devel to the discussion. Please don't add broken misconfigured mailing lists, which require moderator or subscription, to discussions on public lists. Isn't it simpler to ask the spice list maintainer (Alon Levy, CCed) to do it? We cannot have a discussion on a mailing list if we constantly have to wait for the list maintainer to approve it. There is really zero justification for closing a development list of an open source project. Spam is handled in other ways, this doesn't solve it. Cheers, Jes
Re: [Qemu-devel] QEMU: Discussion of separating core functionality vs supportive features
On 03/02/11 11:56, Dor Laor wrote: On 03/02/2011 12:25 PM, Jes Sorensen wrote: On 03/01/11 15:25, Dor Laor wrote: Using shared memory this way should allow us to implement the video clients without performance loss, in fact it should be beneficial since it would allow them to run fully separate from the host daemon. Why do you call it a daemon? Each VM instance should have only one, the 'host daemon' naming is misleading. I refer to it as a daemon because it is something the client(s) will connect to. But yes, there will be a daemon per VM. The proper solution long term is to sandbox qemu in a way that there privileged mode and non privileged mode. It might be implemented using separate address space or not. Most operations like vnc/rpc/spice/usb should be run with less privileges. The main issue is that doing it right will take time and we'll want virt-agent be merged before the long term solution is ready. The best approach would be gradual development Yes I agree, I don't think this will happen overnight, and blocking virtagent with this would be bad. Cheers, Jes
Re: [Qemu-devel] QEMU: Discussion of separating core functionality vs supportive features
On 03/02/11 11:58, Alon Levy wrote: On Wed, Mar 02, 2011 at 11:25:44AM +0100, Jes Sorensen wrote: I had a few thoughts about this already, which I think will work for both spice and vnc. What we could do is to expose the video memory via shared memory. That way a spice or vnc daemon could get direct access to the memory, this would limit communication to keyboard/mouse events, as well as video mode info, and possibly notifications to the client about which ranges of memory have been updated. Using shared memory this way should allow us to implement the video clients without performance loss, in fact it should be beneficial since it would allow them to run fully separate from the host daemon. I think that would work well for spice. Spice uses shared memory from the pci device for both the framebuffer and surfaces/commands, but this is not really relevant at this level. What about IO and irq? that would add additional latencies, no? because each io exit would need to be ipc'ed over to the spice/vnc process? and same way in the other direction, requesting qemu to trigger an interrupt in the next vm entry. I am glad the shmem approach will work for Spice. There would be something there with IRQs etc, I agree. but there are methods to help that too, we could use a shared memory event ring for example. Cheers, Jes
Re: [Qemu-devel] QEMU: Discussion of separating core functionality vs supportive features
On 03/02/11 14:13, Michael Roth wrote: On 03/02/2011 04:19 AM, Jes Sorensen wrote: It is absolutely vital for me that we do not make things much more complicated for users with this move. I don't want to get into a situation where we start forcing external packages or daemons in order to run basic QEMU. If we start requiring such, we have failed! However, I think it is a reasonable compromise to have one daemon you launch, and then a simple client you launch straight after which will then provide the same/similar views and interfaces that users are used to when they launch current QEMU (monitor, vnc server etc). I think the challenge with this approach is defining an IPC mechanism that is robust enough to support it. For instance, on one end of the spectrum we have Spice, where shared memory paired with some mechanism for IO notification between the client/server might make sense. But then we have things like the human monitor where a socket interface or pipe is clearly more the more straight-forward route. We may find that it more desirable to have multiple classes of client components, each contain within a client process with it's own IPC mechanism. Although, multiple IPC mechanisms doesn't sound particularly enticing either...but 2 might not be so unreasonable. Hi Michael, I think we need two types for sure, even for the video case, we will still need a control channel as well. However, I don't think it is desirable to split things up more than we have to, so if we can keep it within one client process that is good. Maybe there are cases where it makes sense to split it into more processes, I could be convinced, but I think we really need to be careful making it too much of a complex mess either. Cheers, Jes
[Qemu-devel] QEMU: Discussion of separating core functionality vs supportive features
Hi, On last week's call we discussed the issue of splitting non core features of QEMU into it's own process to reduce the security risks etc. I wrote up a summary of my thoughts on this to try to cover the various issues. Feedback welcome and hopefully we can continue the discussion on a future call - maybe next week? I would like to be part of the discussion, but it's a public holiday here March 1st, so I won't be on that call. Cheers, Jes Separating host-side virtagent and other tasks from core QEMU = To improve auditing of the core QEMU code, it would be ideal to be able to separate the core QEMU functionality from utility functionality by moving the utility functionality into its own process. This process will be referred to as the QEMU client below. Components which are candidates for moving out of QEMU include: - virtagent - vnc server (and other graphical displays such as SDL, spice and curses) - human monitor The idea is to have QEMU launch as a daemon, and then allow for one of more client processes to connect to it. These will then offer the various services. The main issue to discuss is how to handle various state information, reconnects, and migration. Security The primary reason for this discussion is security, however there are other benefits from this split, as will be mentioned below. During a demo of virtagent I hit a case where a bug in the agent command handling code caused a crash of the host QEMU process. While it is probably a simple bug, it shows how adding more complexity to the QEMU process increases the risk of adding security problems that could potentially be exploited by a hostile guest. By splitting non core functionality into a QEMU client process, the host process will be isolated from a large number of potential security problems, ie. in case a client process is killed or crashes, it should not affect the main QEMU process. In addition it makes it easier to audit the core QEMU functionality. virtagent = In short virtagent provides a set of simple commands, most of which do not have state associated with them. These include shutdown, ping, fsfreeze/fsthaw, etc. Other commands might be multi-stage commands which could fail if the client is disconnected from the daemon while the command is in progress. These include copy-paste and file copy. vnc server == The vnc server simply needs a connection to the video memory of the QEMU process, video mode state, as well as channels for sending keyboard and mouse events. It is stateless by nature and supports reconnects. This applies to the other graphical display engines (SDL, spice, and curses) as well. Human monitor = The human monitor is effectively stateless. It issues commands and prints the result. There is no state in the monitor and it can be built directly on top of QMP. An additional benefit here is that it would allow for multiple monitors. Disconnects === It must be possible for a client process getting killed or disconnected from the QEMU process, in which case is should be possible to launch a new client that connects to the QEMU process. In this case, commands needs to be provided allowing the client process to query the QEMU process and virtagent for current state information. In-progress commands may fail, and will need to be re-run, such as copy-paste and and file copy. However neither of these are vital commands and a re-run of such commands is acceptable behavior. Migration = Given that migration moves the guest to a new QEMU process, normally on a different host, any connection from management tools to the monitor, QMP sockets, virtio-serial, etc. require a new connection through the new QEMU process. Stateless connections, such as the monitor, QMP and the vnc-server handles reconnects without problems, and should not have any issues during migration that are different from the issues in the current implementation. The main issues causing problems are stateful events such as copy-paste, which is handled via a guest agent. The copy-paste problem can be handled by blocking copy-paste operations during migration, until the guest agent is reachable through the QEMU process on the new host. This does mean that copy-paste can block or fail temporarily (depending on whether is it implemented as -EBUSY or just blocks), however it is not a mission critical feature, and it can also block or fail temporarily during normal operation on a non virtual system. Per the nature of the operation, a file copy via a guest agent is going to fail and will have to be restarted after migration has completed, in case it does not complete. This is again a non critical feature and requiring a restart is acceptable.
[Qemu-devel] [PATCH] For AIO return -ENOSPC on short write
From: Jes Sorensen jes.soren...@redhat.com
Signed-off-by: Jes Sorensen jes.soren...@redhat.com
---
linux-aio.c |5 +
1 files changed, 5 insertions(+), 0 deletions(-)
diff --git a/linux-aio.c b/linux-aio.c
index 68f4b3d..d9c0225 100644
--- a/linux-aio.c
+++ b/linux-aio.c
@@ -32,6 +32,7 @@ struct qemu_laiocb {
ssize_t ret;
size_t nbytes;
int async_context_id;
+int type;
QLIST_ENTRY(qemu_laiocb) node;
};
@@ -62,6 +63,9 @@ static void qemu_laio_process_completion(struct
qemu_laio_state *s,
if (ret != -ECANCELED) {
if (ret == laiocb-nbytes)
ret = 0;
+else if ((laiocb-type == QEMU_AIO_WRITE) (ret = 0)
+ (ret laiocb-nbytes))
+ret = -ENOSPC;
else if (ret = 0)
ret = -EINVAL;
@@ -204,6 +208,7 @@ BlockDriverAIOCB *laio_submit(BlockDriverState *bs, void
*aio_ctx, int fd,
laiocb-nbytes = nb_sectors * 512;
laiocb-ctx = s;
laiocb-ret = -EINPROGRESS;
+laiocb-type = type;
laiocb-async_context_id = get_async_context_id();
iocbs = laiocb-iocb;
--
1.7.4
[Qemu-devel] [PATCH] For AIO return -ENOSPC on short write
From: Jes Sorensen jes.soren...@redhat.com Hi, Current AIO code returns -EINVAL on every error, even on write. In order to be able to report some more sensible error messages, I suggest we change it to return -ENOSPC if we are failing on a write request and we had a partial write. It matches more what one would expect. One way to reproduce the strange error return is this: 1) setup a loopback device for a file (say 2GB) 2) create an LVM volume over it 3) create a qcow2 image on top of it which is larger than the 2GB 4) try and install a guest. Comments? any reason why this is a bad idea? Cheers, Jes Jes Sorensen (1): For AIO return -ENOSPC on short write linux-aio.c |5 + 1 files changed, 5 insertions(+), 0 deletions(-) -- 1.7.4
[Qemu-devel] Re: [PATCH] For AIO return -ENOSPC on short write
On 02/22/11 12:44, Kevin Wolf wrote:
@@ -62,6 +63,9 @@ static void qemu_laio_process_completion(struct
qemu_laio_state *s,
if (ret != -ECANCELED) {
if (ret == laiocb-nbytes)
ret = 0;
+else if ((laiocb-type == QEMU_AIO_WRITE) (ret = 0)
+ (ret laiocb-nbytes))
+ret = -ENOSPC;
else if (ret = 0)
ret = -EINVAL;
Isn't there a way to get the real error code instead of just making it
up more cleverly? Like retrying for the rest of the request?
Kevin
I guess we could retry the last part of the request, but if we already
have an error, it seems silly to try to rewrite the same stuff again
just to obtain the error code.
I looked through the aio calls and I didn't find any obvious way to
retrieve the error code, but maybe I missed something?
Cheers,
Jes
Re: [Qemu-devel] [RFC][PATCH v6 00/23] virtagent: host/guest RPC communication agent
On 02/18/11 15:57, Anthony Liguori wrote: On 02/18/2011 08:30 AM, Jes Sorensen wrote: However if there's an agent connection, it could be arranged in a way allowing the host to reconnect to the guest agent. In that way it really shouldn't be a big deal as long as our agent commands aren't too complex. Oh, but they'll be nice and complex :-) What happens if you do a live migration in the middle of doing a live backup? You'll end up with the guest waiting to be told that it's okay to unfreeze itself only to never be told. Well that isn't really different from the current setup - if QEMU migrates, the admin tool has to connect to the new QEMU process and issue the fsthaw command there instead. Terminating in QEMU means we can do intelligent things like delay live migration convergence, save state, etc. We could easily add a flag for this in QEMU itself, so I don't see it being a big issue. Jes
Re: [Qemu-devel] [RFC][PATCH v6 00/23] virtagent: host/guest RPC communication agent
On 02/21/11 14:36, Michael Roth wrote: On 02/21/2011 02:32 AM, Jes Sorensen wrote: Well that isn't really different from the current setup - if QEMU migrates, the admin tool has to connect to the new QEMU process and issue the fsthaw command there instead. Another thing to consider is that virtagent is bi-directional, and may be tracking the state of state-full RPCs on behalf of the guest client, just as the guest daemon may be tracking the state of stateful RPCs on behalf of the host client. We cannot maintain consistent state without migrating the host-side state information along with the guest. What kinda of usages do you expect to need to preserve state like this? It seems a bad solution to me for the guest to be able to rely on state in the host like this. Cheers, Jes
Re: [Qemu-devel] [RFC][PATCH v6 00/23] virtagent: host/guest RPC communication agent
On 02/17/11 15:39, Michael Roth wrote: On 02/17/2011 02:26 AM, Jes Sorensen wrote: On 02/16/11 18:22, Michael Roth wrote: worry is that we are adding a lot of complexity into QEMU on the host side which is going to be difficult to audit, especially with things like the HTML and XML processing. If we separated host side processing into a separate command, we could better protect ourselves against a situation where a rogue guest could kill QEMU and possibly exploit it on the host side. I think we should seriously look at moving the agent processing code out of main QEMU and into a standalone command, maybe qemu-va-host or something like that. I don't think the problem is really so fundamental...if you saw a host-side crash it's most likely a bug/sloppy error-handling in virtagent. Malformed xml (from version mismatches, transports errors, etc) shouldn't crash xmlrpc-c... it's using a libxml parser that just returns an error on unexpected xml...we just need to make sure we handle errors appropriately. Hi Michael, It may not be so fundamental, but it still makes me wary. XMLRPC handling is quite high level and introduces the potential of errors that are outside of our direct control. Personally I don't see the big benefit of having virtagent terminate in QEMU, if anything it actually makes me wary. I would quite like to see the monitor moved out of QEMU as well and into it's own process - the simpler we make QEMU in this regard, the more secure it will be to run. Using either a fork() approach or simply a separate process that connects to the QEMU process seems a very reasonable approach IMHO. Can you provide some details on what you ran and what the error message was? It's a bit tricky, I was running a my tests over VNC on a remote system (think trans-Atlantic latency) while having 10 people watch while I typed the commands. It seemed that pretty much every agent command was causing it, including ping, but unfortunately I didn't save the backtrace. Cheers, Jes
Re: [Qemu-devel] [RFC][PATCH v6 00/23] virtagent: host/guest RPC communication agent
On 02/18/11 15:07, Anthony Liguori wrote: On 02/18/2011 06:45 AM, Jes Sorensen wrote: It may not be so fundamental, but it still makes me wary. XMLRPC handling is quite high level and introduces the potential of errors that are outside of our direct control. Personally I don't see the big benefit of having virtagent terminate in QEMU, Live migration. If it's a separate daemon, then live migration gets fugly. If xmlrpc-c is a PoS, then we ought to look at using something else. But let's understand what's happening first before drawing any conclusions. Urgh, I always do my best to pretend that there is no such thing as live migration :) Never seem to work though :( However if there's an agent connection, it could be arranged in a way allowing the host to reconnect to the guest agent. In that way it really shouldn't be a big deal as long as our agent commands aren't too complex. xmlrpc-c is probably fine, but it introduces a layer of complexity which always makes me worried. Cheers, Jes
Re: [Qemu-devel] [RFC][PATCH v6 00/23] virtagent: host/guest RPC communication agent
On 02/16/11 18:22, Michael Roth wrote: We've seen similar behavior. I think it comes down to qemu-va being linked against shared objects in the host that don't necessarily coincide with what's in the guest. It's somewhat misleading that we currently build qemu-va along with the binary, since qemu-va is not meant to be used on the host, and the version built on the host is not meant to be used in the guest. Really the guest binary, qemu-va, should be built in a proper build environment for that particular guest. Long term it may make sense to have a guest-utils target that isn't part of the normal host-build process to reflect binaries with these kinds of requirements. For now I think we'll may just end up removing qemu-va from the default make target, and only build it explicitly with make qemu-va. Hi Michael, I am not sure I was totally clear in my mail, but the crashes I saw were QEMU on the host that went down. Not qemu-va running in the guest. My worry is that we are adding a lot of complexity into QEMU on the host side which is going to be difficult to audit, especially with things like the HTML and XML processing. If we separated host side processing into a separate command, we could better protect ourselves against a situation where a rogue guest could kill QEMU and possibly exploit it on the host side. I think we should seriously look at moving the agent processing code out of main QEMU and into a standalone command, maybe qemu-va-host or something like that. There has been talk about doing the same thing with the monitor in the past, and have it talk to the main QEMU process over QMP. This pretty much goes along the same lines, except that I think we need the XML handling moved out with it, so we couldn't just layer it directly on top of QMP. P.S. Hoping to have the execute-RPCs-in-seperate-threads work done soon so we can get back to integrating your patches. Sounds good! Cheers, Jes
[Qemu-devel] Re: [PATCH v2] tracetool: Add optional argument to specify dtrace probe names
On 02/17/11 13:16, Paolo Bonzini wrote: On 02/15/2011 01:34 PM, jes.soren...@redhat.com wrote: -if [ -z $binary ]; then +if [ -z $probeprefix -a -z $binary ]; then echo --binary is required for SystemTAP tapset generator exit 1 fi --binary is always required, even with --probe-prefix, since it is used for +probe $probeprefix.$name = process($binary).mark($name) Paolo The patch was acked like this upstream, so if I broke something with this, please note it there too. I didn't think it was necessary, but I'll check. Thanks, Jes
[Qemu-devel] [PATCH v3] tracetool: Add optional argument to specify dtrace probe names
From: Jes Sorensen jes.soren...@redhat.com
Optional feature allowing a user to generate the probe list to match
the name of the binary, in case they wish to install qemu under a
different name than qemu-{system,user},arch
Signed-off-by: Jes Sorensen jes.soren...@redhat.com
---
scripts/tracetool | 19 +--
1 files changed, 13 insertions(+), 6 deletions(-)
diff --git a/scripts/tracetool b/scripts/tracetool
index e046683..412f695 100755
--- a/scripts/tracetool
+++ b/scripts/tracetool
@@ -30,9 +30,11 @@ Output formats:
--stap Generate .stp file (DTrace with SystemTAP only)
Options:
- --binary [path] Full path to QEMU binary
- --target-arch [arch] QEMU emulator target arch
- --target-type [type] QEMU emulator target type ('system' or 'user')
+ --binary [path]Full path to QEMU binary
+ --target-arch [arch]QEMU emulator target arch
+ --target-type [type]QEMU emulator target type ('system' or 'user')
+ --probe-prefix [prefix] Prefix for dtrace probe names
+ (default: qemu-\$targettype-\$targetarch)
EOF
exit 1
@@ -472,7 +474,7 @@ linetostap_dtrace()
# Define prototype for probe arguments
cat EOF
-probe qemu.$targettype.$targetarch.$name = process($binary).mark($name)
+probe $probeprefix.$name = process($binary).mark($name)
{
EOF
@@ -574,14 +576,17 @@ tracetostap()
echo --binary is required for SystemTAP tapset generator
exit 1
fi
-if [ -z $targettype ]; then
+if [ -z $probeprefix -a -z $targettype ]; then
echo --target-type is required for SystemTAP tapset generator
exit 1
fi
-if [ -z $targetarch ]; then
+if [ -z $probeprefix -a -z $targetarch ]; then
echo --target-arch is required for SystemTAP tapset generator
exit 1
fi
+if [ -z $probeprefix ]; then
+ probeprefix=qemu.$targettype.$targetarch;
+fi
echo /* This file is autogenerated by tracetool, do not edit. */
convert stap
}
@@ -592,6 +597,7 @@ output=
binary=
targettype=
targetarch=
+probeprefix=
until [ -z $1 ]
@@ -602,6 +608,7 @@ do
--binary) shift ; binary=$1 ;;
--target-arch) shift ; targetarch=$1 ;;
--target-type) shift ; targettype=$1 ;;
+--probe-prefix) shift ; probeprefix=$1 ;;
-h | -c | -d) output=${1#-} ;;
--stap) output=${1#--} ;;
--
1.7.4
[Qemu-devel] Re: [PATCH v2] tracetool: Add optional argument to specify dtrace probe names
On 02/17/11 13:29, Paolo Bonzini wrote: On 02/17/2011 01:19 PM, Jes Sorensen wrote: On 02/17/11 13:16, Paolo Bonzini wrote: On 02/15/2011 01:34 PM, jes.soren...@redhat.com wrote: -if [ -z $binary ]; then +if [ -z $probeprefix -a -z $binary ]; then echo --binary is required for SystemTAP tapset generator exit 1 fi --binary is always required, even with --probe-prefix, since it is used for +probe $probeprefix.$name = process($binary).mark($name) if I broke something with this No, you didn't break anything. It's just that someone patching the Makefile like you did in 2/2 might. Paolo Sorry for the confusion everybody, I thought I was replying to an internal email - where I mangled a Makefile. Please ignore the noise and consider v3 of the patch. Thanks, Jes
Re: [Qemu-devel] [RFC][PATCH v6 00/23] virtagent: host/guest RPC communication agent
On 01/17/11 14:14, Michael Roth wrote: These patches apply to master (1-14-2011), and can also be obtained from: git://repo.or.cz/qemu/mdroth.git virtagent_v6 CHANGES IN V6: - Added a sentinel value to reliably detect the start of an http hdr. Used to skip past partially sent http content from previous sessions - Added http hdr tag (currently hardcoded for testing, will switch to uuid) to filter out valid-but-unexpected content in channel from previous sessions - Added timeout mechanism to avoid hanging monitor when agent isn't running - Added timed back-off on read's from a virtio-serial that result in ret=0 to avoid spinning if host isn't connected. - Added daemonize flags to qemu-va - Added sane defaults for channel type and virtio-serial port path - Various bug fixes for state machine/job handling logic Hi Michael, I was running some testing here of virtagent and demoing it to some of my colleagues and ran into a problem that raised an interesting question. My test system was an older Fedora 11 system, which meant I had to rebuild qemu, while I kept my test image and the qemu-va binary that I had built on a Fedora 14 system. What happened was that either due to the differences in platform, or maybe due to lag in updating the windows over vnc, agent commands would end up crashing qemu on the host. I am not sure whether this was due to timeouts or incompatibility of the libraries, however the question raised is whether it is good security wise to pull XMLRPC processing into QEMU this way? Instead maybe it would be better to move it out into it's own process that uses virtio-serial through QEMU for it's communication? In addition I think we need to consider a mechanism to make sure that the host and guest side are really compatible. Just a few things to consider. Cheers, Jes
[Qemu-devel] [PATCH] tracetool: Add optional argument to specify dtrace probe names
From: Jes Sorensen jes.soren...@redhat.com
Optional feature allowing a user to generate the probe list to match
the name of the binary, in case they wish to install qemu under a
different name than qemu-{system,user},arch
Signed-off-by: Jes Sorensen jes.soren...@redhat.com
---
scripts/tracetool | 14 ++
1 files changed, 10 insertions(+), 4 deletions(-)
diff --git a/scripts/tracetool b/scripts/tracetool
index e046683..0200afa 100755
--- a/scripts/tracetool
+++ b/scripts/tracetool
@@ -33,6 +33,7 @@ Options:
--binary [path] Full path to QEMU binary
--target-arch [arch] QEMU emulator target arch
--target-type [type] QEMU emulator target type ('system' or 'user')
+ --probe-name [name] Specify alternative name for dtrace probes
EOF
exit 1
@@ -472,7 +473,7 @@ linetostap_dtrace()
# Define prototype for probe arguments
cat EOF
-probe qemu.$targettype.$targetarch.$name = process($binary).mark($name)
+probe $probename.$name = process($binary).mark($name)
{
EOF
@@ -570,18 +571,21 @@ tracetostap()
echo SystemTAP tapset generator not applicable to $backend backend
exit 1
fi
-if [ -z $binary ]; then
+if [ -z $probename -a -z $binary ]; then
echo --binary is required for SystemTAP tapset generator
exit 1
fi
-if [ -z $targettype ]; then
+if [ -z $probename -a -z $targettype ]; then
echo --target-type is required for SystemTAP tapset generator
exit 1
fi
-if [ -z $targetarch ]; then
+if [ -z $probename -a -z $targetarch ]; then
echo --target-arch is required for SystemTAP tapset generator
exit 1
fi
+if [ -z $probename ]; then
+ probename=qemu.$targettype.$targetarch;
+fi
echo /* This file is autogenerated by tracetool, do not edit. */
convert stap
}
@@ -592,6 +596,7 @@ output=
binary=
targettype=
targetarch=
+probename=
until [ -z $1 ]
@@ -602,6 +607,7 @@ do
--binary) shift ; binary=$1 ;;
--target-arch) shift ; targetarch=$1 ;;
--target-type) shift ; targettype=$1 ;;
+--probe-name) shift ; probename=$1 ;;
-h | -c | -d) output=${1#-} ;;
--stap) output=${1#--} ;;
--
1.7.4
[Qemu-devel] [PATCH v2] tracetool: Add optional argument to specify dtrace probe names
From: Jes Sorensen jes.soren...@redhat.com
Optional feature allowing a user to generate the probe list to match
the name of the binary, in case they wish to install qemu under a
different name than qemu-{system,user},arch
Signed-off-by: Jes Sorensen jes.soren...@redhat.com
---
scripts/tracetool | 21 ++---
1 files changed, 14 insertions(+), 7 deletions(-)
diff --git a/scripts/tracetool b/scripts/tracetool
index e046683..b4d74ec 100755
--- a/scripts/tracetool
+++ b/scripts/tracetool
@@ -30,9 +30,11 @@ Output formats:
--stap Generate .stp file (DTrace with SystemTAP only)
Options:
- --binary [path] Full path to QEMU binary
- --target-arch [arch] QEMU emulator target arch
- --target-type [type] QEMU emulator target type ('system' or 'user')
+ --binary [path]Full path to QEMU binary
+ --target-arch [arch]QEMU emulator target arch
+ --target-type [type]QEMU emulator target type ('system' or 'user')
+ --probe-prefix [prefix] Prefix for dtrace probe names
+ (default: qemu-\$targettype-\$targetarch)
EOF
exit 1
@@ -472,7 +474,7 @@ linetostap_dtrace()
# Define prototype for probe arguments
cat EOF
-probe qemu.$targettype.$targetarch.$name = process($binary).mark($name)
+probe $probeprefix.$name = process($binary).mark($name)
{
EOF
@@ -570,18 +572,21 @@ tracetostap()
echo SystemTAP tapset generator not applicable to $backend backend
exit 1
fi
-if [ -z $binary ]; then
+if [ -z $probeprefix -a -z $binary ]; then
echo --binary is required for SystemTAP tapset generator
exit 1
fi
-if [ -z $targettype ]; then
+if [ -z $probeprefix -a -z $targettype ]; then
echo --target-type is required for SystemTAP tapset generator
exit 1
fi
-if [ -z $targetarch ]; then
+if [ -z $probeprefix -a -z $targetarch ]; then
echo --target-arch is required for SystemTAP tapset generator
exit 1
fi
+if [ -z $probeprefix ]; then
+ probeprefix=qemu.$targettype.$targetarch;
+fi
echo /* This file is autogenerated by tracetool, do not edit. */
convert stap
}
@@ -592,6 +597,7 @@ output=
binary=
targettype=
targetarch=
+probeprefix=
until [ -z $1 ]
@@ -602,6 +608,7 @@ do
--binary) shift ; binary=$1 ;;
--target-arch) shift ; targetarch=$1 ;;
--target-type) shift ; targettype=$1 ;;
+--probe-prefix) shift ; probeprefix=$1 ;;
-h | -c | -d) output=${1#-} ;;
--stap) output=${1#--} ;;
--
1.7.4
[Qemu-devel] [PATCH] Change snapshot_blkdev hmp to use correct argument type for device
From: Jes Sorensen jes.soren...@redhat.com
Pointed out by Markus
Signed-off-by: Jes Sorensen jes.soren...@redhat.com
---
hmp-commands.hx |2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/hmp-commands.hx b/hmp-commands.hx
index 38e1eb7..372bef4 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -822,7 +822,7 @@ ETEXI
{
.name = snapshot_blkdev,
-.args_type = device:s,snapshot_file:s?,format:s?,
+.args_type = device:B,snapshot_file:s?,format:s?,
.params = device [new-image-file] [format],
.help = initiates a live snapshot\n\t\t\t
of device. If a new image file is specified,
the\n\t\t\t
--
1.7.3.5
[Qemu-devel] [PATCH v3 0/2] virtagent - fsfreeze support
From: Jes Sorensen jes.soren...@redhat.com Hi This is a first attempt to add fsfreeze support to virtagent. The idea is for the guest agent to walk the list of locally mounted file systems in the guest, and issuing an ioctl to freeze them. The host can then do a live snapshot of the guest, obtaining stable file systems. After the snapshot, the host then calls the thaw function in virtagent, which goes through the list of previously frozen file systems and unfreezes them. The list walking ignores remote file systems such as NFS and CIFS as well as all pseudo file systems. The guest agent code is in the first patch, and host agent code is in the second patch. For now there is only human monitor support, but it should be pretty straight forward to add QMP support as well. Comments and suggestions welcome! v3 of the patch encapsulates the freeze states in a struct and fixes some tab issues that I had missed. Both pointed out by Michael Roth. v2 of the patch addresses the issues pointed out by Stefan and Michael. Note I will be gone all of next week, so if I don't reply it's because I am busy skiing :) Cheers, Jes Jes Sorensen (2): Add virtagent file system freeze/thaw Add monitor commands for fsfreeze support hmp-commands.hx| 48 +++ virtagent-common.h |9 ++ virtagent-server.c | 195 +++ virtagent.c| 235 virtagent.h|9 ++ 5 files changed, 496 insertions(+), 0 deletions(-) -- 1.7.3.5
Re: [Qemu-devel] [PATCH 1/2] Add virtagent file system freeze/thaw
On 02/03/11 18:41, Michael Roth wrote: On 02/02/2011 02:48 AM, Jes Sorensen wrote: Yes very much so[1] - one reason why it would be nice to have virtagent use threads to execute the actual commands. We should probably add a flag to agent commands indicating whether they issue disk I/O or not, so we can block attempts to execute commands that do so, while the guest is frozen. **Warning, epic response** For things like logging and i/o on a frozen system...I agree we'd need some flag for these kinds of situations. Maybe a disable_logging() flagi really don't like this though... I'd imagine even syslogd() could block virtagent in this type of situation, so that would need to be disabled as well. One way to resolve this would be to have the logging handled in it's own thread, which uses non blocking calls to do the actual logging. Obviously we'd have to use a non file system based communication method between the main thread and the logging thread :) But doing so completely subverts our attempts and providing proper accounting of what the agent is doing to the user. A user can freeze the filesystem, knowing that logging would be disabled, then prod at whatever he wants. So the handling should be something specific to fsfreeze, with stricter requirements: If a user calls fsfreeze(), we disable logging, but also disable the ability to do anything other than fsthaw() or fsstatus(). This actually solves the potential deadlocking problem for other RPCs as well...since they cant be executed in the first place. I disagree that we should block all calls, except for fsfreeze/fsstatus/ fsthaw in this case. There are other calls that could be valid in this situations, so I think it needs to be evaluated on a case by case basis. So a solution for these situations is still needed, and I'm starting to agree that threads are needed, but I don't think we should do RPCs concurrently (not sure if that's what is being suggested or not). At least, there's no pressing reason for it as things currently stand (there aren't currently any RPCs where fast response times are all that important, so it's okay to serialize them behind previous RPCs, and HMP/QMP are command at a time), and it's something that Im fairly confident can be added if the need arises in the future. Eventually I think we will need to be able to support concurrent RPC calls. There can be situations where an operation takes a long time while it is valid to be able to ping the guest agent to verify that it is still alive etc. Cheers, Jes
[Qemu-devel] [PATCH 2/2] Add monitor commands for fsfreeze support
From: Jes Sorensen jes.soren...@redhat.com
This patch adds the following monitor commands:
agent_fsfreeze:
- Freezes all local file systems in the guest. Command will print
the number of file systems that were frozen.
agent_fsthaw:
- Thaws all local file systems in the guest. Command will print
the number of file systems that were thawed.
agent_fsstatus:
- Prints the current status of file systems in the guest:
Thawed, frozen, thaw in progress, freeze in progress, error.
Signed-off-by: Jes Sorensen jes.soren...@redhat.com
---
hmp-commands.hx| 48 +++
virtagent-common.h |1 +
virtagent.c| 235
virtagent.h|9 ++
4 files changed, 293 insertions(+), 0 deletions(-)
diff --git a/hmp-commands.hx b/hmp-commands.hx
index 9c7ac0b..f4150da 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -1310,6 +1310,54 @@ STEXI
Fetch and re-negotiate guest agent capabilties
ETEXI
+{
+.name = agent_fsfreeze,
+.args_type = ,
+.params = ,
+.help = Freeze all local file systems mounted in the guest,
+.user_print = do_agent_fsfreeze_print,
+.mhandler.cmd_async = do_agent_fsfreeze,
+.flags = MONITOR_CMD_ASYNC,
+},
+
+STEXI
+@item agent_fsfreeze
+@findex agent_fsfreeze
+Freeze all local mounted file systems in guest
+ETEXI
+
+{
+.name = agent_fsthaw,
+.args_type = ,
+.params = ,
+.help = Thaw all local file systems mounted in the guest,
+.user_print = do_agent_fsthaw_print,
+.mhandler.cmd_async = do_agent_fsthaw,
+.flags = MONITOR_CMD_ASYNC,
+},
+
+STEXI
+@item agent_fsthaw
+@findex agent_fsthaw
+Thaw all local mounted file systems in guest
+ETEXI
+
+{
+.name = agent_fsstatus,
+.args_type = ,
+.params = ,
+.help = Display status of file system freeze progress in guest,
+.user_print = do_agent_fsstatus_print,
+.mhandler.cmd_async = do_agent_fsstatus,
+.flags = MONITOR_CMD_ASYNC,
+},
+
+STEXI
+@item agent_fsstatus
+@findex agent_fsstatus
+Get status of file system freeze in guest
+ETEXI
+
STEXI
@end table
ETEXI
diff --git a/virtagent-common.h b/virtagent-common.h
index 7c6d9ef..ff7bf23 100644
--- a/virtagent-common.h
+++ b/virtagent-common.h
@@ -24,6 +24,7 @@
#include monitor.h
#include virtagent-server.h
#include virtagent.h
+#include qint.h
#define DEBUG_VA
diff --git a/virtagent.c b/virtagent.c
index b5e7944..4277802 100644
--- a/virtagent.c
+++ b/virtagent.c
@@ -640,3 +640,238 @@ int va_send_hello(void)
xmlrpc_DECREF(params);
return ret;
}
+
+void do_agent_fsfreeze_print(Monitor *mon, const QObject *data)
+{
+TRACE(called);
+
+monitor_printf(mon, File systems frozen: % PRId64 \n,
+ qint_get_int((qobject_to_qint(data;
+}
+
+static void do_agent_fsfreeze_cb(const char *resp_data,
+ size_t resp_data_len,
+ MonitorCompletion *mon_cb,
+ void *mon_data)
+{
+xmlrpc_value *resp = NULL;
+xmlrpc_env env;
+xmlrpc_int32 retval = 0;
+QInt *qint;
+
+TRACE(called);
+
+if (resp_data == NULL) {
+LOG(error handling RPC request);
+return;
+}
+
+xmlrpc_env_init(env);
+resp = xmlrpc_parse_response(env, resp_data, resp_data_len);
+if (va_rpc_has_error(env)) {
+LOG(error parsing RPC response);
+return;
+}
+
+xmlrpc_parse_value(env, resp, i, retval);
+if (va_rpc_has_error(env)) {
+retval = -1;
+goto out;
+}
+
+out:
+qint = qint_from_int(retval);
+xmlrpc_DECREF(resp);
+if (mon_cb) {
+mon_cb(mon_data, QOBJECT(qint));
+}
+qobject_decref(QOBJECT(qint));
+}
+
+int do_agent_fsfreeze(Monitor *mon, const QDict *mon_params,
+ MonitorCompletion cb, void *opaque)
+{
+xmlrpc_env env;
+xmlrpc_value *params;
+int ret;
+
+TRACE(called);
+
+xmlrpc_env_init(env);
+params = xmlrpc_build_value(env, ());
+if (va_rpc_has_error(env)) {
+return -1;
+}
+
+ret = va_do_rpc(env, va.fsfreeze, params, do_agent_fsfreeze_cb,
+cb, opaque);
+if (ret) {
+qerror_report(QERR_VA_FAILED, ret, strerror(ret));
+}
+xmlrpc_DECREF(params);
+return ret;
+}
+
+void do_agent_fsthaw_print(Monitor *mon, const QObject *data)
+{
+TRACE(called);
+
+monitor_printf(mon, File systems thawed: % PRId64 \n,
+ qint_get_int((qobject_to_qint(data;
+}
+
+static void do_agent_fsthaw_cb(const char *resp_data,
+ size_t resp_data_len,
+ MonitorCompletion *mon_cb,
+ void *mon_data)
+{
+xmlrpc_value *resp = NULL;
+xmlrpc_env env;
+xmlrpc_int32
[Qemu-devel] Re: [PATCH 1/2] Add virtagent file system freeze/thaw
On 02/03/11 19:11, Michael Roth wrote:
@@ -217,6 +221,186 @@ static xmlrpc_value *va_hello(xmlrpc_env *env,
return result;
}
+
+/*
+ * Walk the mount table and build a list of local file systems
+ */
+
+struct direntry {
+char *dirname;
+char *devtype;
+struct direntry *next;
+};
+
+static struct direntry *va_mount_list;
+static int va_fsfreeze_status;
And what I meant in the last RFC about using objects was to
encapsulate global state information for a particular group of commands
in single data type/variable. We're gonna end up with a similar set of
variables for stateful RPCs like copyfile and potentially a few for
things like spice. So to avoid having things get too cluttered up I'd
prefer something like, in this particular case:
typedef struct VAFSFreezeState {
struct direntry *mount_list;
int status;
} VAFSFeezeState;
static VAFSFreezeState va_fsfreeze_state;
Ok, I got rid of the tabs (damn I thought I had caught them all), and
added a struct to keep the freeze state. I didn't add any typedef
grossness though.
v3 coming up.
Cheers,
Jes
[Qemu-devel] [PATCH 1/2] Add virtagent file system freeze/thaw
From: Jes Sorensen jes.soren...@redhat.com
Implement freeze/thaw support in the guest, allowing the host to
request the guest freezes all it's file systems before a live snapshot
is performed.
- fsfreeze(): Walk the list of mounted local real file systems,
and freeze them.
- fsthaw(): Walk the list of previously frozen file systems and
thaw them.
- fsstatus(): Return the current status of freeze/thaw. The host must
poll this function, in case fsfreeze() returned with a
timeout, to wait for the operation to finish.
Signed-off-by: Jes Sorensen jes.soren...@redhat.com
---
virtagent-common.h |8 ++
virtagent-server.c | 195
2 files changed, 203 insertions(+), 0 deletions(-)
diff --git a/virtagent-common.h b/virtagent-common.h
index 5d8f5c1..7c6d9ef 100644
--- a/virtagent-common.h
+++ b/virtagent-common.h
@@ -61,6 +61,14 @@ typedef struct VAContext {
const char *channel_path;
} VAContext;
+enum va_fsfreeze_status {
+FREEZE_ERROR = -1,
+FREEZE_THAWED = 0,
+FREEZE_INPROGRESS = 1,
+FREEZE_FROZEN = 2,
+FREEZE_THAWINPROGRESS = 3,
+};
+
enum va_job_status {
VA_JOB_STATUS_PENDING = 0,
VA_JOB_STATUS_OK,
diff --git a/virtagent-server.c b/virtagent-server.c
index 7bb35b2..5140918 100644
--- a/virtagent-server.c
+++ b/virtagent-server.c
@@ -14,6 +14,10 @@
#include syslog.h
#include qemu_socket.h
#include virtagent-common.h
+#include mntent.h
+#include sys/types.h
+#include sys/ioctl.h
+#include linux/fs.h
static VAServerData *va_server_data;
static bool va_enable_syslog = false; /* enable syslog'ing of RPCs */
@@ -217,6 +221,191 @@ static xmlrpc_value *va_hello(xmlrpc_env *env,
return result;
}
+
+/*
+ * Walk the mount table and build a list of local file systems
+ */
+
+struct direntry {
+char *dirname;
+char *devtype;
+struct direntry *next;
+};
+
+struct va_freezestate {
+struct direntry *mount_list;
+int status;
+};
+
+static struct va_freezestate freezestate;
+
+static int build_mount_list(void)
+{
+struct mntent *mnt;
+struct direntry *entry;
+struct direntry *next;
+char const *mtab = MOUNTED;
+FILE *fp;
+
+fp = setmntent(mtab, r);
+if (!fp) {
+fprintf(stderr, unable to read mtab\n);
+goto fail;
+}
+
+while ((mnt = getmntent(fp))) {
+/*
+ * An entry which device name doesn't start with a '/' is
+ * either a dummy file system or a network file system.
+ * Add special handling for smbfs and cifs as is done by
+ * coreutils as well.
+ */
+if ((mnt-mnt_fsname[0] != '/') ||
+(strcmp(mnt-mnt_type, smbfs) == 0) ||
+(strcmp(mnt-mnt_type, cifs) == 0)) {
+continue;
+}
+
+entry = qemu_malloc(sizeof(struct direntry));
+entry-dirname = qemu_strdup(mnt-mnt_dir);
+entry-devtype = qemu_strdup(mnt-mnt_type);
+entry-next = freezestate.mount_list;
+
+freezestate.mount_list = entry;
+}
+
+endmntent(fp);
+
+return 0;
+
+fail:
+while(freezestate.mount_list) {
+next = freezestate.mount_list-next;
+qemu_free(freezestate.mount_list-dirname);
+qemu_free(freezestate.mount_list-devtype);
+qemu_free(freezestate.mount_list);
+freezestate.mount_list = next;
+}
+
+return -1;
+}
+
+/*
+ * va_fsfreeze(): Walk list of mounted file systems in the guest, and
+ * freeze the ones which are real local file systems.
+ * rpc return values: Number of file systems frozen, -1 on error.
+ */
+static xmlrpc_value *va_fsfreeze(xmlrpc_env *env,
+ xmlrpc_value *params,
+ void *user_data)
+{
+xmlrpc_int32 ret = 0, i = 0;
+xmlrpc_value *result;
+struct direntry *entry;
+int fd;
+SLOG(va_fsfreeze());
+
+if (freezestate.status != FREEZE_THAWED) {
+ret = 0;
+goto out;
+}
+
+ret = build_mount_list();
+if (ret 0) {
+goto out;
+}
+
+freezestate.status = FREEZE_INPROGRESS;
+
+entry = freezestate.mount_list;
+while(entry) {
+fd = qemu_open(entry-dirname, O_RDONLY);
+if (fd == -1) {
+ret = errno;
+goto error;
+}
+ret = ioctl(fd, FIFREEZE);
+close(fd);
+if (ret 0 ret != EOPNOTSUPP) {
+goto error;
+}
+
+entry = entry-next;
+i++;
+}
+
+freezestate.status = FREEZE_FROZEN;
+ret = i;
+out:
+result = xmlrpc_build_value(env, i, ret);
+return result;
+error:
+if (i 0) {
+freezestate.status = FREEZE_ERROR;
+}
+goto out;
+}
+
+/*
+ * va_fsthaw(): Walk list of frozen file systems in the guest, and
+ * thaw them.
+ * rpc return values: Number of file systems thawed on success, -1 on error.
+ */
+static xmlrpc_value *va_fsthaw(xmlrpc_env *env
[Qemu-devel] [PATCH 1/2] Add virtagent file system freeze/thaw
From: Jes Sorensen jes.soren...@redhat.com
Implement freeze/thaw support in the guest, allowing the host to
request the guest freezes all it's file systems before a live snapshot
is performed.
- fsfreeze(): Walk the list of mounted local real file systems,
and freeze them.
- fsthaw(): Walk the list of previously frozen file systems and
thaw them.
- fsstatus(): Return the current status of freeze/thaw. The host must
poll this function, in case fsfreeze() returned with a
timeout, to wait for the operation to finish.
Signed-off-by: Jes Sorensen jes.soren...@redhat.com
---
virtagent-common.h |8 ++
virtagent-server.c | 190
2 files changed, 198 insertions(+), 0 deletions(-)
diff --git a/virtagent-common.h b/virtagent-common.h
index 5d8f5c1..7c6d9ef 100644
--- a/virtagent-common.h
+++ b/virtagent-common.h
@@ -61,6 +61,14 @@ typedef struct VAContext {
const char *channel_path;
} VAContext;
+enum va_fsfreeze_status {
+FREEZE_ERROR = -1,
+FREEZE_THAWED = 0,
+FREEZE_INPROGRESS = 1,
+FREEZE_FROZEN = 2,
+FREEZE_THAWINPROGRESS = 3,
+};
+
enum va_job_status {
VA_JOB_STATUS_PENDING = 0,
VA_JOB_STATUS_OK,
diff --git a/virtagent-server.c b/virtagent-server.c
index 7bb35b2..ffbe163 100644
--- a/virtagent-server.c
+++ b/virtagent-server.c
@@ -14,6 +14,10 @@
#include syslog.h
#include qemu_socket.h
#include virtagent-common.h
+#include mntent.h
+#include sys/types.h
+#include sys/ioctl.h
+#include linux/fs.h
static VAServerData *va_server_data;
static bool va_enable_syslog = false; /* enable syslog'ing of RPCs */
@@ -217,6 +221,186 @@ static xmlrpc_value *va_hello(xmlrpc_env *env,
return result;
}
+
+/*
+ * Walk the mount table and build a list of local file systems
+ */
+
+struct direntry {
+char *dirname;
+char *devtype;
+struct direntry *next;
+};
+
+static struct direntry *va_mount_list;
+static int va_fsfreeze_status;
+
+static int build_mount_list(void)
+{
+struct mntent *mnt;
+struct direntry *entry;
+struct direntry *next;
+char const *mtab = MOUNTED;
+FILE *fp;
+
+fp = setmntent(mtab, r);
+if (!fp) {
+ fprintf(stderr, unable to read mtab\n);
+ goto fail;
+}
+
+while ((mnt = getmntent(fp))) {
+ /*
+* An entry which device name doesn't start with a '/' is
+* either a dummy file system or a network file system.
+* Add special handling for smbfs and cifs as is done by
+* coreutils as well.
+*/
+ if ((mnt-mnt_fsname[0] != '/') ||
+ (strcmp(mnt-mnt_type, smbfs) == 0) ||
+ (strcmp(mnt-mnt_type, cifs) == 0)) {
+ continue;
+ }
+
+ entry = qemu_malloc(sizeof(struct direntry));
+ entry-dirname = qemu_strdup(mnt-mnt_dir);
+ entry-devtype = qemu_strdup(mnt-mnt_type);
+ entry-next = va_mount_list;
+
+ va_mount_list = entry;
+}
+
+endmntent(fp);
+
+return 0;
+
+fail:
+while(va_mount_list) {
+ next = va_mount_list-next;
+qemu_free(va_mount_list-dirname);
+qemu_free(va_mount_list-devtype);
+qemu_free(va_mount_list);
+va_mount_list = next;
+}
+
+return -1;
+}
+
+/*
+ * va_fsfreeze(): Walk list of mounted file systems in the guest, and
+ * freeze the ones which are real local file systems.
+ * rpc return values: Number of file systems frozen, -1 on error.
+ */
+static xmlrpc_value *va_fsfreeze(xmlrpc_env *env,
+ xmlrpc_value *params,
+ void *user_data)
+{
+xmlrpc_int32 ret = 0, i = 0;
+xmlrpc_value *result;
+struct direntry *entry;
+int fd;
+SLOG(va_fsfreeze());
+
+if (va_fsfreeze_status != FREEZE_THAWED) {
+ret = 0;
+goto out;
+}
+
+ret = build_mount_list();
+if (ret 0) {
+goto out;
+}
+
+va_fsfreeze_status = FREEZE_INPROGRESS;
+
+entry = va_mount_list;
+while(entry) {
+fd = qemu_open(entry-dirname, O_RDONLY);
+if (fd == -1) {
+ret = errno;
+goto error;
+}
+ret = ioctl(fd, FIFREEZE);
+close(fd);
+if (ret 0 ret != EOPNOTSUPP) {
+goto error;
+}
+
+entry = entry-next;
+i++;
+}
+
+va_fsfreeze_status = FREEZE_FROZEN;
+ret = i;
+out:
+result = xmlrpc_build_value(env, i, ret);
+return result;
+error:
+if (i 0) {
+va_fsfreeze_status = FREEZE_ERROR;
+}
+goto out;
+}
+
+/*
+ * va_fsthaw(): Walk list of frozen file systems in the guest, and
+ * thaw them.
+ * rpc return values: Number of file systems thawed on success, -1 on error.
+ */
+static xmlrpc_value *va_fsthaw(xmlrpc_env *env,
+ xmlrpc_value *params,
+ void *user_data)
+{
+xmlrpc_int32 ret;
+xmlrpc_value *result
[Qemu-devel] [PATCH 2/2] Add monitor commands for fsfreeze support
From: Jes Sorensen jes.soren...@redhat.com
This patch adds the following monitor commands:
agent_fsfreeze:
- Freezes all local file systems in the guest. Command will print
the number of file systems that were frozen.
agent_fsthaw:
- Thaws all local file systems in the guest. Command will print
the number of file systems that were thawed.
agent_fsstatus:
- Prints the current status of file systems in the guest:
Thawed, frozen, thaw in progress, freeze in progress, error.
Signed-off-by: Jes Sorensen jes.soren...@redhat.com
---
hmp-commands.hx| 48 +++
virtagent-common.h |1 +
virtagent.c| 235
virtagent.h|9 ++
4 files changed, 293 insertions(+), 0 deletions(-)
diff --git a/hmp-commands.hx b/hmp-commands.hx
index 9c7ac0b..f4150da 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -1310,6 +1310,54 @@ STEXI
Fetch and re-negotiate guest agent capabilties
ETEXI
+{
+.name = agent_fsfreeze,
+.args_type = ,
+.params = ,
+.help = Freeze all local file systems mounted in the guest,
+.user_print = do_agent_fsfreeze_print,
+.mhandler.cmd_async = do_agent_fsfreeze,
+.flags = MONITOR_CMD_ASYNC,
+},
+
+STEXI
+@item agent_fsfreeze
+@findex agent_fsfreeze
+Freeze all local mounted file systems in guest
+ETEXI
+
+{
+.name = agent_fsthaw,
+.args_type = ,
+.params = ,
+.help = Thaw all local file systems mounted in the guest,
+.user_print = do_agent_fsthaw_print,
+.mhandler.cmd_async = do_agent_fsthaw,
+.flags = MONITOR_CMD_ASYNC,
+},
+
+STEXI
+@item agent_fsthaw
+@findex agent_fsthaw
+Thaw all local mounted file systems in guest
+ETEXI
+
+{
+.name = agent_fsstatus,
+.args_type = ,
+.params = ,
+.help = Display status of file system freeze progress in guest,
+.user_print = do_agent_fsstatus_print,
+.mhandler.cmd_async = do_agent_fsstatus,
+.flags = MONITOR_CMD_ASYNC,
+},
+
+STEXI
+@item agent_fsstatus
+@findex agent_fsstatus
+Get status of file system freeze in guest
+ETEXI
+
STEXI
@end table
ETEXI
diff --git a/virtagent-common.h b/virtagent-common.h
index 7c6d9ef..ff7bf23 100644
--- a/virtagent-common.h
+++ b/virtagent-common.h
@@ -24,6 +24,7 @@
#include monitor.h
#include virtagent-server.h
#include virtagent.h
+#include qint.h
#define DEBUG_VA
diff --git a/virtagent.c b/virtagent.c
index b5e7944..4277802 100644
--- a/virtagent.c
+++ b/virtagent.c
@@ -640,3 +640,238 @@ int va_send_hello(void)
xmlrpc_DECREF(params);
return ret;
}
+
+void do_agent_fsfreeze_print(Monitor *mon, const QObject *data)
+{
+TRACE(called);
+
+monitor_printf(mon, File systems frozen: % PRId64 \n,
+ qint_get_int((qobject_to_qint(data;
+}
+
+static void do_agent_fsfreeze_cb(const char *resp_data,
+ size_t resp_data_len,
+ MonitorCompletion *mon_cb,
+ void *mon_data)
+{
+xmlrpc_value *resp = NULL;
+xmlrpc_env env;
+xmlrpc_int32 retval = 0;
+QInt *qint;
+
+TRACE(called);
+
+if (resp_data == NULL) {
+LOG(error handling RPC request);
+return;
+}
+
+xmlrpc_env_init(env);
+resp = xmlrpc_parse_response(env, resp_data, resp_data_len);
+if (va_rpc_has_error(env)) {
+LOG(error parsing RPC response);
+return;
+}
+
+xmlrpc_parse_value(env, resp, i, retval);
+if (va_rpc_has_error(env)) {
+retval = -1;
+goto out;
+}
+
+out:
+qint = qint_from_int(retval);
+xmlrpc_DECREF(resp);
+if (mon_cb) {
+mon_cb(mon_data, QOBJECT(qint));
+}
+qobject_decref(QOBJECT(qint));
+}
+
+int do_agent_fsfreeze(Monitor *mon, const QDict *mon_params,
+ MonitorCompletion cb, void *opaque)
+{
+xmlrpc_env env;
+xmlrpc_value *params;
+int ret;
+
+TRACE(called);
+
+xmlrpc_env_init(env);
+params = xmlrpc_build_value(env, ());
+if (va_rpc_has_error(env)) {
+return -1;
+}
+
+ret = va_do_rpc(env, va.fsfreeze, params, do_agent_fsfreeze_cb,
+cb, opaque);
+if (ret) {
+qerror_report(QERR_VA_FAILED, ret, strerror(ret));
+}
+xmlrpc_DECREF(params);
+return ret;
+}
+
+void do_agent_fsthaw_print(Monitor *mon, const QObject *data)
+{
+TRACE(called);
+
+monitor_printf(mon, File systems thawed: % PRId64 \n,
+ qint_get_int((qobject_to_qint(data;
+}
+
+static void do_agent_fsthaw_cb(const char *resp_data,
+ size_t resp_data_len,
+ MonitorCompletion *mon_cb,
+ void *mon_data)
+{
+xmlrpc_value *resp = NULL;
+xmlrpc_env env;
+xmlrpc_int32
[Qemu-devel] Re: [PATCH 1/2] Add virtagent file system freeze/thaw
On 02/01/11 17:50, Michael Roth wrote:
On 02/01/2011 04:58 AM, jes.soren...@redhat.com wrote:
+enum vs_fsfreeze_status {
+FREEZE_ERROR = -1,
+FREEZE_THAWED = 0,
+FREEZE_INPROGRESS = 1,
+FREEZE_FROZEN = 2,
+FREEZE_THAWINPROGRESS = 3,
+};
Any reason for vs_* vs. va_*?
H let me see if I can find a good excuse for that typo :)
diff --git a/virtagent-server.c b/virtagent-server.c
index 7bb35b2..cf2a3f0 100644
--- a/virtagent-server.c
+++ b/virtagent-server.c
@@ -14,6 +14,13 @@
#includesyslog.h
#include qemu_socket.h
#include virtagent-common.h
+#includemntent.h
+#includesys/types.h
+#includesys/stat.h
+#includesys/errno.h
+#includesys/ioctl.h
+#includefcntl.h
+#includelinux/fs.h
Can probably clean these up a bit, I believe fcntl.h/errno.h/stat.h are
already available at least.
Carry-over from writing the code outside of qemu. Would be much cleaner
than relying on the include everything and the kitchen sink in a global
header file, but thats how it is :(
+
+fsfreeze_status = FREEZE_INPROGRESS;
+
+entry = mount_list;
I think as we start adding more and more stateful RPCs, free-floating
state variables can start getting a bit hairy to keep track of.
Eventually I'd like to have state information that only applies to a
subset of RPCs consolidated into a single object. I wouldn't focus on
this too much because I'd like to have an interface to do this in the
future (mainly so they can state objects can register themselves and
provide a reset() function that can be called when, for instance, an
agent disconnects/reconnects), but in the meantime I think it would be
more readable to have a global va_fsfreeze_state object to track freeze
status and mount points.
Urgh, what do you mean by object here? I have to admit the word object
always makes me cringe I changed the variables to have the va_ prefix.
+static xmlrpc_value *va_fsstatus(xmlrpc_env *env,
+ xmlrpc_value *params,
+ void *user_data)
+{
+xmlrpc_value *result = xmlrpc_build_value(env, i,
fsfreeze_status);
+SLOG(va_fsstatus());
+return result;
+}
Hmm, you mentioned before that these freezes may be long-running
jobs...do the ioctl()'s not return until completion? There is global
timeout in virtagent, currently under a minute, to prevent a virtagent
monitor command from hanging the monitor session, so if it's unlikely
you'll fit in this window we'll need to work on something to better
support these this kinds of situations.
I think 1 minute is fine, but we should probably look at something a
little more flexible for handling commands over the longer term. Maybe
have virtagent spawn threads for executing some commands?
The 3 main approaches would be:
1) allow command-specific timeouts with values that are sane for the
command in question, and potentially allow timeouts to be disabled
2) fork() long running jobs and provide a mechanism for them to provide
asynchronous updates to us to we can query status
3) fork() long running jobs, have them provide status information
elsewhere, and provide a polling function to check that status
3) would likely require something like writing status to a file and then
provide a polling function to check it, which doesn't work here so
that's probably out.
I'd initially planned on doing 2) at some point, but I'm beginning to
think 1) is the better approach, since qemu opts in on how long it's
willing to hang for a particular command, so there's not really any
surprises. At least not to qemu...users might get worried after a while,
so there is a bit of a trade-off. But it's also more user-friendlyno
need for polling or dealing with asynchronous updates to figure out when
an RPC has actually finished. Seem reasonable?
I am not sure which is really the best solution. Basically we will need
to classify commands into two categories, so if you issue a certain type
of command, like agent_fsfreeze() (basically when the agent is in
FREEZE_FROZEN state) only status commands are allowed to execute in
parallel. Anything that tries to issue a write to the file system will
hang until agent_fsthaw is called. However it would be useful to be able
to call in for non-blocking status commands etc.
I'll post a v2 in a minute that addresses the issues pointed out by
Stefan and you. I think the threading/timeout aspect is something we
need to look at for the longer term.
Cheers,
Jes
[Qemu-devel] [PATCH v2 0/2] virtagent - fsfreeze support
From: Jes Sorensen jes.soren...@redhat.com Hi This is a first attempt to add fsfreeze support to virtagent. The idea is for the guest agent to walk the list of locally mounted file systems in the guest, and issuing an ioctl to freeze them. The host can then do a live snapshot of the guest, obtaining stable file systems. After the snapshot, the host then calls the thaw function in virtagent, which goes through the list of previously frozen file systems and unfreezes them. The list walking ignores remote file systems such as NFS and CIFS as well as all pseudo file systems. The guest agent code is in the first patch, and host agent code is in the second patch. For now there is only human monitor support, but it should be pretty straight forward to add QMP support as well. Comments and suggestions welcome! v2 of the patch addresses the issues pointed out by Stefan and Michael. Cheers, Jes Jes Sorensen (2): Add virtagent file system freeze/thaw Add monitor commands for fsfreeze support hmp-commands.hx| 48 +++ virtagent-common.h |9 ++ virtagent-server.c | 190 ++ virtagent.c| 235 virtagent.h|9 ++ 5 files changed, 491 insertions(+), 0 deletions(-) -- 1.7.3.5
Re: [Qemu-devel] [PATCH 1/2] Add virtagent file system freeze/thaw
On 02/02/11 08:57, Stefan Hajnoczi wrote: On Tue, Feb 1, 2011 at 10:58 AM, jes.soren...@redhat.com wrote: From: Jes Sorensen jes.soren...@redhat.com Implement freeze/thaw support in the guest, allowing the host to request the guest freezes all it's file systems before a live snapshot is performed. - fsfreeze(): Walk the list of mounted local real file systems, and freeze them. Does this add a requirement that guest agent code issues no disk I/O in its main loop (e.g. logging)? Otherwise we might deadlock ourselves waiting for I/O which is never issued. Yes very much so[1] - one reason why it would be nice to have virtagent use threads to execute the actual commands. We should probably add a flag to agent commands indicating whether they issue disk I/O or not, so we can block attempts to execute commands that do so, while the guest is frozen. Cheers, Jes [1] speaking from experience ... a Linux desktop gets really upset if you freeze the file systems from a command in an xterm ho hum
[Qemu-devel] [PATCH 0/2] virtagent - fsfreeze support
From: Jes Sorensen jes.soren...@redhat.com Hi This is a first attempt to add fsfreeze support to virtagent. The idea is for the guest agent to walk the list of locally mounted file systems in the guest, and issuing an ioctl to freeze them. The host can then do a live snapshot of the guest, obtaining stable file systems. After the snapshot, the host then calls the thaw function in virtagent, which goes through the list of previously frozen file systems and unfreezes them. The list walking ignores remote file systems such as NFS and CIFS as well as all pseudo file systems. The guest agent code is in the first patch, and host agent code is in the second patch. For now there is only human monitor support, but it should be pretty straight forward to add QMP support as well. Patches are against the virtagent-dev git tree. Comments and suggestions welcome! Cheers, Jes Jes Sorensen (2): Add virtagent file system freeze/thaw Add monitor commands for fsfreeze support hmp-commands.hx| 48 +++ virtagent-common.h |9 ++ virtagent-server.c | 196 +++ virtagent.c| 235 virtagent.h|9 ++ 5 files changed, 497 insertions(+), 0 deletions(-) -- 1.7.3.5
[Qemu-devel] [PATCH 1/2] Add virtagent file system freeze/thaw
From: Jes Sorensen jes.soren...@redhat.com
Implement freeze/thaw support in the guest, allowing the host to
request the guest freezes all it's file systems before a live snapshot
is performed.
- fsfreeze(): Walk the list of mounted local real file systems,
and freeze them.
- fsthaw(): Walk the list of previously frozen file systems and
thaw them.
- fsstatus(): Return the current status of freeze/thaw. The host must
poll this function, in case fsfreeze() returned with a
timeout, to wait for the operation to finish.
Signed-off-by: Jes Sorensen jes.soren...@redhat.com
---
virtagent-common.h |8 ++
virtagent-server.c | 196
2 files changed, 204 insertions(+), 0 deletions(-)
diff --git a/virtagent-common.h b/virtagent-common.h
index 5d8f5c1..220a4b6 100644
--- a/virtagent-common.h
+++ b/virtagent-common.h
@@ -61,6 +61,14 @@ typedef struct VAContext {
const char *channel_path;
} VAContext;
+enum vs_fsfreeze_status {
+FREEZE_ERROR = -1,
+FREEZE_THAWED = 0,
+FREEZE_INPROGRESS = 1,
+FREEZE_FROZEN = 2,
+FREEZE_THAWINPROGRESS = 3,
+};
+
enum va_job_status {
VA_JOB_STATUS_PENDING = 0,
VA_JOB_STATUS_OK,
diff --git a/virtagent-server.c b/virtagent-server.c
index 7bb35b2..cf2a3f0 100644
--- a/virtagent-server.c
+++ b/virtagent-server.c
@@ -14,6 +14,13 @@
#include syslog.h
#include qemu_socket.h
#include virtagent-common.h
+#include mntent.h
+#include sys/types.h
+#include sys/stat.h
+#include sys/errno.h
+#include sys/ioctl.h
+#include fcntl.h
+#include linux/fs.h
static VAServerData *va_server_data;
static bool va_enable_syslog = false; /* enable syslog'ing of RPCs */
@@ -217,6 +224,189 @@ static xmlrpc_value *va_hello(xmlrpc_env *env,
return result;
}
+
+/*
+ * Walk the mount table and build a list of local file systems
+ */
+
+struct direntry {
+char *dirname;
+char *devtype;
+struct direntry *next;
+};
+
+static struct direntry *mount_list;
+static int fsfreeze_status;
+
+static int build_mount_list(void)
+{
+struct mntent *mnt;
+struct direntry *entry;
+struct direntry *next;
+char const *mtab = MOUNTED;
+FILE *fp;
+
+fp = setmntent(mtab, r);
+if (!fp) {
+ fprintf(stderr, unable to read mtab\n);
+ goto fail;
+}
+
+while ((mnt = getmntent(fp))) {
+ /*
+* An entry which device name doesn't start with a '/' is
+* either a dummy file system or a network file system.
+* Add special handling for smbfs and cifs as is done by
+* coreutils as well.
+*/
+ if ((mnt-mnt_fsname[0] != '/') ||
+ (strcmp(mnt-mnt_type, smbfs) == 0) ||
+ (strcmp(mnt-mnt_type, cifs) == 0)) {
+ continue;
+ }
+
+ entry = qemu_malloc(sizeof(struct direntry));
+ if (!entry) {
+ goto fail;
+ }
+ entry-dirname = qemu_strdup(mnt-mnt_dir);
+ entry-devtype = qemu_strdup(mnt-mnt_type);
+ entry-next = mount_list;
+
+ mount_list = entry;
+}
+
+endmntent(fp);
+
+return 0;
+
+fail:
+while(mount_list) {
+ next = mount_list-next;
+ qemu_free(mount_list-dirname);
+ qemu_free(mount_list-devtype);
+ qemu_free(mount_list);
+ mount_list = next;
+}
+
+return -1;
+}
+
+/*
+ * va_fsfreeze(): Walk list of mounted file systems in the guest, and
+ * freeze the ones which are real local file systems.
+ * rpc return values: Number of file systems frozen, -1 on error.
+ */
+static xmlrpc_value *va_fsfreeze(xmlrpc_env *env,
+ xmlrpc_value *params,
+ void *user_data)
+{
+xmlrpc_int32 ret = 0, i = 0;
+xmlrpc_value *result;
+struct direntry *entry;
+int fd;
+SLOG(va_fsfreeze());
+
+if (fsfreeze_status == FREEZE_FROZEN) {
+ret = 0;
+goto out;
+}
+
+ret = build_mount_list();
+if (ret 0) {
+goto out;
+}
+
+fsfreeze_status = FREEZE_INPROGRESS;
+
+entry = mount_list;
+while(entry) {
+fd = qemu_open(entry-dirname, O_RDONLY);
+if (fd == -1) {
+ret = errno;
+goto error;
+}
+ret = ioctl(fd, FIFREEZE);
+if (ret 0 ret != EOPNOTSUPP) {
+goto error;
+}
+
+close(fd);
+entry = entry-next;
+i++;
+}
+
+fsfreeze_status = FREEZE_FROZEN;
+ret = i;
+out:
+result = xmlrpc_build_value(env, i, ret);
+return result;
+error:
+if (i 0) {
+fsfreeze_status = FREEZE_ERROR;
+}
+goto out;
+}
+
+/*
+ * va_fsthaw(): Walk list of frozen file systems in the guest, and
+ * thaw them.
+ * rpc return values: Number of file systems thawed on success, -1 on error.
+ */
+static xmlrpc_value *va_fsthaw(xmlrpc_env *env,
+ xmlrpc_value *params
[Qemu-devel] [PATCH 2/2] Add monitor commands for fsfreeze support
From: Jes Sorensen jes.soren...@redhat.com
This patch adds the following monitor commands:
agent_fsfreeze:
- Freezes all local file systems in the guest. Command will print
the number of file systems that were frozen.
agent_fsthaw:
- Thaws all local file systems in the guest. Command will print
the number of file systems that were thawed.
agent_fsstatus:
- Prints the current status of file systems in the guest:
Thawed, frozen, thaw in progress, freeze in progress, error.
Signed-off-by: Jes Sorensen jes.soren...@redhat.com
---
hmp-commands.hx| 48 +++
virtagent-common.h |1 +
virtagent.c| 235
virtagent.h|9 ++
4 files changed, 293 insertions(+), 0 deletions(-)
diff --git a/hmp-commands.hx b/hmp-commands.hx
index 9c7ac0b..f4150da 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -1310,6 +1310,54 @@ STEXI
Fetch and re-negotiate guest agent capabilties
ETEXI
+{
+.name = agent_fsfreeze,
+.args_type = ,
+.params = ,
+.help = Freeze all local file systems mounted in the guest,
+.user_print = do_agent_fsfreeze_print,
+.mhandler.cmd_async = do_agent_fsfreeze,
+.flags = MONITOR_CMD_ASYNC,
+},
+
+STEXI
+@item agent_fsfreeze
+@findex agent_fsfreeze
+Freeze all local mounted file systems in guest
+ETEXI
+
+{
+.name = agent_fsthaw,
+.args_type = ,
+.params = ,
+.help = Thaw all local file systems mounted in the guest,
+.user_print = do_agent_fsthaw_print,
+.mhandler.cmd_async = do_agent_fsthaw,
+.flags = MONITOR_CMD_ASYNC,
+},
+
+STEXI
+@item agent_fsthaw
+@findex agent_fsthaw
+Thaw all local mounted file systems in guest
+ETEXI
+
+{
+.name = agent_fsstatus,
+.args_type = ,
+.params = ,
+.help = Display status of file system freeze progress in guest,
+.user_print = do_agent_fsstatus_print,
+.mhandler.cmd_async = do_agent_fsstatus,
+.flags = MONITOR_CMD_ASYNC,
+},
+
+STEXI
+@item agent_fsstatus
+@findex agent_fsstatus
+Get status of file system freeze in guest
+ETEXI
+
STEXI
@end table
ETEXI
diff --git a/virtagent-common.h b/virtagent-common.h
index 220a4b6..383de84 100644
--- a/virtagent-common.h
+++ b/virtagent-common.h
@@ -24,6 +24,7 @@
#include monitor.h
#include virtagent-server.h
#include virtagent.h
+#include qint.h
#define DEBUG_VA
diff --git a/virtagent.c b/virtagent.c
index b5e7944..4277802 100644
--- a/virtagent.c
+++ b/virtagent.c
@@ -640,3 +640,238 @@ int va_send_hello(void)
xmlrpc_DECREF(params);
return ret;
}
+
+void do_agent_fsfreeze_print(Monitor *mon, const QObject *data)
+{
+TRACE(called);
+
+monitor_printf(mon, File systems frozen: % PRId64 \n,
+ qint_get_int((qobject_to_qint(data;
+}
+
+static void do_agent_fsfreeze_cb(const char *resp_data,
+ size_t resp_data_len,
+ MonitorCompletion *mon_cb,
+ void *mon_data)
+{
+xmlrpc_value *resp = NULL;
+xmlrpc_env env;
+xmlrpc_int32 retval = 0;
+QInt *qint;
+
+TRACE(called);
+
+if (resp_data == NULL) {
+LOG(error handling RPC request);
+return;
+}
+
+xmlrpc_env_init(env);
+resp = xmlrpc_parse_response(env, resp_data, resp_data_len);
+if (va_rpc_has_error(env)) {
+LOG(error parsing RPC response);
+return;
+}
+
+xmlrpc_parse_value(env, resp, i, retval);
+if (va_rpc_has_error(env)) {
+retval = -1;
+goto out;
+}
+
+out:
+qint = qint_from_int(retval);
+xmlrpc_DECREF(resp);
+if (mon_cb) {
+mon_cb(mon_data, QOBJECT(qint));
+}
+qobject_decref(QOBJECT(qint));
+}
+
+int do_agent_fsfreeze(Monitor *mon, const QDict *mon_params,
+ MonitorCompletion cb, void *opaque)
+{
+xmlrpc_env env;
+xmlrpc_value *params;
+int ret;
+
+TRACE(called);
+
+xmlrpc_env_init(env);
+params = xmlrpc_build_value(env, ());
+if (va_rpc_has_error(env)) {
+return -1;
+}
+
+ret = va_do_rpc(env, va.fsfreeze, params, do_agent_fsfreeze_cb,
+cb, opaque);
+if (ret) {
+qerror_report(QERR_VA_FAILED, ret, strerror(ret));
+}
+xmlrpc_DECREF(params);
+return ret;
+}
+
+void do_agent_fsthaw_print(Monitor *mon, const QObject *data)
+{
+TRACE(called);
+
+monitor_printf(mon, File systems thawed: % PRId64 \n,
+ qint_get_int((qobject_to_qint(data;
+}
+
+static void do_agent_fsthaw_cb(const char *resp_data,
+ size_t resp_data_len,
+ MonitorCompletion *mon_cb,
+ void *mon_data)
+{
+xmlrpc_value *resp = NULL;
+xmlrpc_env env;
+xmlrpc_int32
Re: [Qemu-devel] [PATCH 0/2] virtagent - fsfreeze support
On 02/01/11 12:25, Vasiliy G Tolstov wrote: Hello. Very nice feature. Sorry for offropic, but can this feature can be used to modify partiotion table on already mounted device (for example root on ext3? ) Thank You. No it cannot, that would be a totally different issue that wouldn't be affected by this. Cheers, Jes
Re: [Qemu-devel] [PATCH 1/2] Add virtagent file system freeze/thaw
On 02/01/11 15:12, Stefan Hajnoczi wrote:
On Tue, Feb 1, 2011 at 10:58 AM, jes.soren...@redhat.com wrote:
From: Jes Sorensen jes.soren...@redhat.com
Implement freeze/thaw support in the guest, allowing the host to
request the guest freezes all it's file systems before a live snapshot
is performed.
- fsfreeze(): Walk the list of mounted local real file systems,
and freeze them.
- fsthaw(): Walk the list of previously frozen file systems and
thaw them.
- fsstatus(): Return the current status of freeze/thaw. The host must
poll this function, in case fsfreeze() returned with a
timeout, to wait for the operation to finish.
It is desirable to minimize the freeze time, which may interrupt or
degrade the service that applications inside the VM can provide.
Polling means we have to choose a fixed value (500 ms?) at which to
check for freeze completion. In this example we could have up to 500
ms extra time spent in freeze because it completed right after we
polled. Any thoughts on this?
I have to admit you lost me here, where do you get that 500ms time from?
Is that the XMLRPC polling time or? I just used the example code from
other agent calls.
In terms of the fsfreeze(), fsthaw(), fsstatus() API, are you looking
at Windows Volume Shadow Copy Services and does this API fit that
model (I haven't looked at it in detail yet)?
http://msdn.microsoft.com/en-us/library/bb968832(v=vs.85).aspx
I haven't looked at it, I designed the calls based on how they fit with
the Linux ioctls.
+ entry = qemu_malloc(sizeof(struct direntry));
+ if (!entry) {
+ goto fail;
+ }
qemu_malloc() never fails.
Good point, we have ugly malloc in qemu :( I wrote the code to handle
this outside QEMU first, to make sure it worked correctly and trying to
see how many times I could crash my laptop in the process. I'll fix it.
+static xmlrpc_value *va_fsfreeze(xmlrpc_env *env,
+ xmlrpc_value *params,
+ void *user_data)
+{
+xmlrpc_int32 ret = 0, i = 0;
+xmlrpc_value *result;
+struct direntry *entry;
+int fd;
+SLOG(va_fsfreeze());
+
+if (fsfreeze_status == FREEZE_FROZEN) {
+ret = 0;
+goto out;
+}
The only valid status here is FREEZE_THAWED? Perhaps we should test
for that specifically.
Good point, I'll fix this.
+
+ret = build_mount_list();
+if (ret 0) {
+goto out;
+}
+
+fsfreeze_status = FREEZE_INPROGRESS;
+
+entry = mount_list;
+while(entry) {
+fd = qemu_open(entry-dirname, O_RDONLY);
+if (fd == -1) {
+ret = errno;
+goto error;
+}
+ret = ioctl(fd, FIFREEZE);
If you close(fd) here then it won't leak or need extra code in the error path.
Good point, will fix.
+static xmlrpc_value *va_fsthaw(xmlrpc_env *env,
+ xmlrpc_value *params,
+ void *user_data)
+{
+xmlrpc_int32 ret;
+xmlrpc_value *result;
+struct direntry *entry;
+int fd, i = 0;
+SLOG(va_fsthaw());
+
+if (fsfreeze_status == FREEZE_THAWED) {
+ret = 0;
+goto out;
+}
A stricter check would be status FREEZE_FROZEN.
Yep, will fix
+
+while((entry = mount_list)) {
+fd = qemu_open(entry-dirname, O_RDONLY);
+if (fd == -1) {
+ret = -1;
+goto out;
+}
+ret = ioctl(fd, FITHAW);
Same thing about close(fd) here.
Thanks for the review, all valid points!
Cheers,
Jes
Re: [Qemu-devel] [PATCH 0/2] virtagent - fsfreeze support
On 02/01/11 15:16, Stefan Hajnoczi wrote: On Tue, Feb 1, 2011 at 10:58 AM, jes.soren...@redhat.com wrote: From: Jes Sorensen jes.soren...@redhat.com This is a first attempt to add fsfreeze support to virtagent. The idea is for the guest agent to walk the list of locally mounted file systems in the guest, and issuing an ioctl to freeze them. The host can then do a live snapshot of the guest, obtaining stable file systems. After the snapshot, the host then calls the thaw function in virtagent, which goes through the list of previously frozen file systems and unfreezes them. Any plans for a call-out to pre/post freeze and thaw scripts so that applications can flush cached data to disk and brace themselves for freeze? Michael and I were discussing this earlier, we need to add it somehow. It could be done as call-outs from the freeze call, or from separate agent calls. Cheers, Jes
Re: [Qemu-devel] [PATCH 1/2] Add virtagent file system freeze/thaw
On 02/01/11 15:34, Stefan Hajnoczi wrote: On Tue, Feb 1, 2011 at 2:26 PM, Jes Sorensen jes.soren...@redhat.com wrote: I have to admit you lost me here, where do you get that 500ms time from? Is that the XMLRPC polling time or? I just used the example code from other agent calls. 500 ms is made up. I was thinking, what would a reasonable polling interval be? and picked a sub-second number. Can you explain how the timeout in fsfreeze can happen? It's probably because I don't know the virtagent details. Ah ok. From what I understand, the XMLRPC code is setup to timeout if the guest doesn't reply within a certain amount of time. In that case, the caller needs to poll to wait for the guest to complete the freeze. This really should only happen if you have a guest with a large number of very large file systems. I don't know how likely it is to happen in real life. Cheers, Jes
[Qemu-devel] Re: [PATCH 1/2] Add virtagent file system freeze/thaw
On 02/01/11 15:48, Adam Litke wrote:
On Tue, 2011-02-01 at 11:58 +0100, jes.soren...@redhat.com wrote:
+/*
+ * va_fsfreeze(): Walk list of mounted file systems in the guest, and
+ * freeze the ones which are real local file systems.
+ * rpc return values: Number of file systems frozen, -1 on error.
+ */
+static xmlrpc_value *va_fsfreeze(xmlrpc_env *env,
+ xmlrpc_value *params,
+ void *user_data)
+{
+xmlrpc_int32 ret = 0, i = 0;
+xmlrpc_value *result;
+struct direntry *entry;
+int fd;
+SLOG(va_fsfreeze());
+
+if (fsfreeze_status == FREEZE_FROZEN) {
+ret = 0;
+goto out;
+}
+
+ret = build_mount_list();
+if (ret 0) {
+goto out;
+}
+
+fsfreeze_status = FREEZE_INPROGRESS;
+
+entry = mount_list;
+while(entry) {
+fd = qemu_open(entry-dirname, O_RDONLY);
+if (fd == -1) {
+ret = errno;
+goto error;
+}
+ret = ioctl(fd, FIFREEZE);
+if (ret 0 ret != EOPNOTSUPP) {
+goto error;
+}
Here we silently ignore filesystems that do not support the FIFREEZE
ioctl. Do we need to have a more complex return value so that we can
communicate which mount points could not be frozen? Otherwise, an
unsuspecting host could retrieve a corrupted snapshot of that
filesystem, right?
That is correct, however most Linux file systems do support it, and for
the ones that don't, there really isn't anything we can do.
Cheers,
Jes
[Qemu-devel] [PATCH] Make spice dummy functions inline to fix calls not checking return values
From: Jes Sorensen jes.soren...@redhat.com
qemu_spice_set_passwd() and qemu_spice_set_pw_expire() dummy functions
needs to be inline, in order to handle the case where they are called
without checking the return value.
Signed-off-by: Jes Sorensen jes.soren...@redhat.com
---
ui/qemu-spice.h | 12 ++--
1 files changed, 10 insertions(+), 2 deletions(-)
diff --git a/ui/qemu-spice.h b/ui/qemu-spice.h
index 48239c3..920d501 100644
--- a/ui/qemu-spice.h
+++ b/ui/qemu-spice.h
@@ -42,8 +42,16 @@ void do_info_spice(Monitor *mon, QObject **ret_data);
#else /* CONFIG_SPICE */
#define using_spice 0
-#define qemu_spice_set_passwd(_p, _f1, _f2) (-1)
-#define qemu_spice_set_pw_expire(_e) (-1)
+static inline int qemu_spice_set_passwd(const char *passwd,
+bool fail_if_connected,
+bool disconnect_if_connected)
+{
+return -1;
+}
+static inline int qemu_spice_set_pw_expire(time_t expires)
+{
+return -1;
+}
#endif /* CONFIG_SPICE */
--
1.7.3.5
[Qemu-devel] [PATCH] Reorganize struct Qcow2Cache for better struct packing
From: Jes Sorensen jes.soren...@redhat.com
Move size after the two pointers in struct Qcow2Cache to get better
packing of struct elements on 64 bit architectures.
Signed-off-by: Jes Sorensen jes.soren...@redhat.com
---
block/qcow2-cache.c |2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/block/qcow2-cache.c b/block/qcow2-cache.c
index 8f2955b..3824739 100644
--- a/block/qcow2-cache.c
+++ b/block/qcow2-cache.c
@@ -35,9 +35,9 @@ typedef struct Qcow2CachedTable {
} Qcow2CachedTable;
struct Qcow2Cache {
-int size;
Qcow2CachedTable* entries;
struct Qcow2Cache* depends;
+int size;
booldepends_on_flush;
boolwritethrough;
};
--
1.7.3.5
Re: [Qemu-devel] [PATCH 1/5] Fast Virtual Disk (FVD) Proposal Part 1
On 01/24/11 15:50, Chunqiang Tang wrote: I think the root of the problem is that your series didn't maintain bisectability. IOW, each patch needs to be able to be applied one at a time such that at each point, the build doesn't break and functionality doesn't break. Otherwise, tools like git bisect don't work. This was true with the old, big FVD patches you reviewed. Following Christoph Hellwig's suggestion, the new series of FVD patches submitted last Friday, e.g., FVD: Added the simulated 'blksim' driver, add individual smaller functions and breaks neither compilation nor execution. Then you need to break up the new patch into smaller chunks and make sure they can each be applied without breaking the build. Having a separate patch that moves functions to another file, because they are too be shared, is a completely valid patch. Jes
[Qemu-devel] [PATCH] Add documentation for STRTOSZ_DEFSUFFIX_ macros
From: Jes Sorensen jes.soren...@redhat.com Signed-off-by: Jes Sorensen jes.soren...@redhat.com --- qemu-common.h |7 +++ 1 files changed, 7 insertions(+), 0 deletions(-) diff --git a/qemu-common.h b/qemu-common.h index c766b99..505e576 100644 --- a/qemu-common.h +++ b/qemu-common.h @@ -153,6 +153,13 @@ int qemu_fls(int i); int qemu_fdatasync(int fd); int fcntl_setfl(int fd, int flag); +/* + * strtosz() suffixes used to specify the default treatment of an + * argument passed to strtosz() without an explicit suffix. + * These should be defined using upper case characters in the range + * A-Z, as strtosz() will use qemu_toupper() on the given argument + * prior to comparison. + */ #define STRTOSZ_DEFSUFFIX_TB 'T' #define STRTOSZ_DEFSUFFIX_GB 'G' #define STRTOSZ_DEFSUFFIX_MB 'M' -- 1.7.3.4
Re: [Qemu-devel] [PATCH 4/4] strtosz(): Use suffix macros in switch() statement
On 01/24/11 18:47, Markus Armbruster wrote: Jes Sorensen jes.soren...@redhat.com writes: qemu_toupper() - whats the problem? If a STRTOSZ_DEFSUFFIX_T? expands to a lower case character, its case will not match any input. Right, so one has to be careful when adding new suffix constants. Calls for a comment right next to the definition of the STRTOSZ_DEFSUFFIX_T*. I hate unstated restrictions that are hidden far away from the place where you can break them. Well I am fine with a comment in the code. However given that we already have all the likely to be used ones for the near future, that isn't exactly a big issue. On the other hand forcing the use of the macros makes it less likely that someone specifies an unsupported constant by hitting 'y' instead of 't' or similar. Takes a combination of butterfingers, cross-eyedness, and near-total incompetence at basic smoke-testing. Not really, all it takes is someone writing a piece of code, not thinking about it, therefore only testing things where a suffix is specified as an argument and it gets missed. Jes
Re: [Qemu-devel] Re: [PATCH v4 0/4] strtosz() cleanups
On 01/24/11 18:28, Stefan Weil wrote: There was some discussion regarding this patch set. I agree with Markus that part of the first patch should be removed: don't change char to unsigned char. The unsigned char should definitely go in, leaving it as a signed char doesn't serve any purpose. It's not necessary, and the result is, that now unsigned chars are assigned to chars which might raise future compiler warnings. We already do that cast by calling qemu_toupper() and qemu_isspace() so that argument is void. Jes
Re: [Qemu-devel] Re: [PATCH v4 0/4] strtosz() cleanups
On 01/25/11 11:14, Markus Armbruster wrote: Jes Sorensen jes.soren...@redhat.com writes: On 01/24/11 18:28, Stefan Weil wrote: There was some discussion regarding this patch set. I agree with Markus that part of the first patch should be removed: don't change char to unsigned char. The unsigned char should definitely go in, leaving it as a signed char doesn't serve any purpose. Leaving something as is doesn't need justification. Changing it does. The justification presented so far was it is prettier to match the real behavior of pure toupper(). Which I don't buy. But without commit access, I'm not a buyer. Well that is just too bad. qemu_toupper() is a hack around the fact that people often forget to use the right type, it is not an excuse for using the wrong type in the code. Jes
Re: [Qemu-devel] [PATCH 4/4] strtosz(): Use suffix macros in switch() statement
On 01/25/11 11:17, Markus Armbruster wrote: Jes Sorensen jes.soren...@redhat.com writes: On 01/24/11 18:47, Markus Armbruster wrote: Jes Sorensen jes.soren...@redhat.com writes: qemu_toupper() - whats the problem? If a STRTOSZ_DEFSUFFIX_T? expands to a lower case character, its case will not match any input. Right, so one has to be careful when adding new suffix constants. Calls for a comment right next to the definition of the STRTOSZ_DEFSUFFIX_T*. I hate unstated restrictions that are hidden far away from the place where you can break them. Well I am fine with a comment in the code. Such a comment improves it from wrong to merely ugly. I can live with that. I realize that you view it as such, in other eyes it goes from hackish to correct ... there is zero point in arguing over this, we can just agree to disagree. Jes
[Qemu-devel] QEMU/KVM Call notes for Jan 25, 2011
Hello, Please find attached my notes from the weekly QEMU/KVM call January 25, 2011. My apologies if I got something wrong. Jes - QEMU 0.14/0.15 releases - Feb 1st 2011 branch to stable tree for 0.14. Anthony would like to do a formal release quickly, so hopefully within 1-2 weeks after snapshot (Feb 15, 2011). - We should plan a formal release schedule for 0.15 http://wiki.qemu.org/Planning/0.15-example - coroutines - Proposal from Stefan Hajnoczi http://www.mail-archive.com/qemu-devel@nongnu.org/msg52522.html - First example is to try and calls to synchronous system calls in QCOW2 codebase. - Suggest every image format in block layer have it's own coroutine layer. - Long term, can/should we apply this to entire block layer? Q: Can we go to real threads in the block layer? A: coroutines are light weight, and we have more control, but it is an option. Going to threads would require a serious amount of work making the full QEMU stack thread safe. Doesn't solve the parallelism problem, so not the final long term solution to the scalability problem. Q: Should we do coroutines in the block layer rather than the image formats? This would provide aio support for formats that do not currently implement aio calls. A: Avi suggests mixed approach, to benefit from not having explicit synchronization points in high performance formats. Follow-on longer discussion about implementation details, which is to be followed up in email. - glib integration - Proposal from Anthony http://www.mail-archive.com/qemu-devel@nongnu.org/msg52798.html - Provides portable interfaces to threading, data structures, and utility functions. - Idea would be to get rid of a lot of duplicated code which we would get from glib instead. - Also consider gio and gobject. - gobject could be a replacement for qdev - gobject could help make vnc server become standalone - Not suggesting to convert everything to glib types (gint, guint, etc). Clarification of which types are recommended will be needed in CODING_STYLE Q: Which would be the first subsystem to convert A: Convert QEMU threads to gthreads - Comment, doing a conversion of QMP would be good, but Luiz will not have time to look at this for maybe 2 months. - Suggestion from Paolo Bonzini to make a standalone library for JSON, rather than relying on the glib implementation - Google Summer of Code 2011 - please post suggestions to the list, and we should revisit this item next week.
[Qemu-devel] Re: [PATCH] block: Use backing format driver during image creation
On 01/24/11 10:32, Stefan Hajnoczi wrote: The backing format should be honored during image creation. For some reason we currently use the image format to open the backing file. This fails when the backing file has a different format than the image being created. Keep the image and backing format drivers completely separate. Also print the backing filename if there is an error opening the backing file instead of the image filename. Signed-off-by: Stefan Hajnoczi stefa...@linux.vnet.ibm.com --- block.c |8 +--- 1 files changed, 5 insertions(+), 3 deletions(-) Looks good! Acked-by: Jes Sorensen jes.soren...@redhat.com
[Qemu-devel] Re: [RFC][PATCH v6 23/23] virtagent: various bits to build QEMU with virtagent
On 01/17/11 14:15, Michael Roth wrote: Signed-off-by: Michael Roth mdr...@linux.vnet.ibm.com --- Makefile.target |2 +- configure | 32 2 files changed, 33 insertions(+), 1 deletions(-) Please make building qemu-va optional, so the build doesn't break if one doesn't have xmlrpc-devel installed. Cheers, Jes
[Qemu-devel] Re: [RFC][PATCH v6 07/23] virtagent: base server definitions
On 01/21/11 18:55, Michael Roth wrote: On 01/21/2011 10:38 AM, Jes Sorensen wrote: +#includexmlrpc-c/base.h +#includexmlrpc-c/server.h + +#define GUEST_AGENT_SERVICE_ID virtagent +#define GUEST_AGENT_PATH /tmp/virtagent-guest.sock +#define HOST_AGENT_SERVICE_ID virtagent-host +#define HOST_AGENT_PATH /tmp/virtagent-host.sock +#define VA_GETFILE_MAX 1 30 +#define VA_FILEBUF_LEN 16384 +#define VA_DMESG_LEN 16384 I really don't like these hard coded constants - you you have a command line interface allowing for the change of the sockets and file names? Otherwise you'll hit problems on the host side with concurrent runs of qemu. Yup, that's one of the TODOs. In terms of configuration we can add parameters to the chardev to override these, but the goal here is sane defaults to avoid unnecessarily complicated invocations. As a sane default, using name.pid or something along those lines is better. It is very common to run more than one qemu instance at a time. I really would like to see the dmesg stuff removed too for now as we discussed earlier. I think as a development/support tool it has a recently strong use case, even given it's limitations (which are not so badwe retrieve up to a max of 16KB, possibly less depending on guest configuration, so it's not entirely predictable, but it's not dangerous. It's platform-specific, but that's handled by capabilities negotiation). There is plenty of good ways to do the same thing, copy file to host, then view is just as easy and can be scripted, without the security implications of having it inline. I just don't really see the downside to keeping it in. It's obviously contentious, and it is not core functionality. In order to get the patches adapted upstream it would easy the process to remove it and keep it as a separate patch. Cheers, Jes
[Qemu-devel] [PATCH v4 0/4] strtosz() cleanups
From: Jes Sorensen jes.soren...@redhat.com Hi, Here is an updated version of the strtosz() fixes that were discussed earlier. Per discussing with Anthony on irc, he is ok with them going in like this. This is a respin to fix a patch conflict in the block tree, and it pulls the two patch sets into one set of four patches instead. No functional change. Cheers, Jes Jes Sorensen (4): strtosz(): use unsigned char and switch to qemu_isspace() strtosz() use qemu_toupper() to simplify switch statement strtosz(): Fix name confusion in use of modf() strtosz(): Use suffix macros in switch() statement cutils.c | 28 1 files changed, 12 insertions(+), 16 deletions(-) -- 1.7.3.4
[Qemu-devel] [PATCH 2/4] strtosz() use qemu_toupper() to simplify switch statement
From: Jes Sorensen jes.soren...@redhat.com
Signed-off-by: Jes Sorensen jes.soren...@redhat.com
---
cutils.c |7 +--
1 files changed, 1 insertions(+), 6 deletions(-)
diff --git a/cutils.c b/cutils.c
index a067cf4..78d35e2 100644
--- a/cutils.c
+++ b/cutils.c
@@ -323,16 +323,14 @@ int64_t strtosz_suffix(const char *nptr, char **end,
const char default_suffix)
d = c;
}
}
-switch (d) {
+switch (qemu_toupper(d)) {
case 'B':
-case 'b':
mul = 1;
if (mul_required) {
goto fail;
}
break;
case 'K':
-case 'k':
mul = 1 10;
break;
case 0:
@@ -340,15 +338,12 @@ int64_t strtosz_suffix(const char *nptr, char **end,
const char default_suffix)
goto fail;
}
case 'M':
-case 'm':
mul = 1ULL 20;
break;
case 'G':
-case 'g':
mul = 1ULL 30;
break;
case 'T':
-case 't':
mul = 1ULL 40;
break;
default:
--
1.7.3.4
[Qemu-devel] [PATCH 1/4] strtosz(): use unsigned char and switch to qemu_isspace()
From: Jes Sorensen jes.soren...@redhat.com
isspace() behavior is undefined for signed char.
Bug pointed out by Eric Blake, thanks!
Signed-off-by: Jes Sorensen jes.soren...@redhat.com
---
cutils.c |7 ---
1 files changed, 4 insertions(+), 3 deletions(-)
diff --git a/cutils.c b/cutils.c
index 4d2e27c..a067cf4 100644
--- a/cutils.c
+++ b/cutils.c
@@ -294,7 +294,8 @@ int fcntl_setfl(int fd, int flag)
int64_t strtosz_suffix(const char *nptr, char **end, const char default_suffix)
{
int64_t retval = -1;
-char *endptr, c, d;
+char *endptr;
+unsigned char c, d;
int mul_required = 0;
double val, mul, integral, fraction;
@@ -314,7 +315,7 @@ int64_t strtosz_suffix(const char *nptr, char **end, const
char default_suffix)
*/
c = *endptr;
d = c;
-if (isspace(c) || c == '\0' || c == ',') {
+if (qemu_isspace(c) || c == '\0' || c == ',') {
c = 0;
if (default_suffix) {
d = default_suffix;
@@ -361,7 +362,7 @@ int64_t strtosz_suffix(const char *nptr, char **end, const
char default_suffix)
*/
if (c != 0) {
endptr++;
-if (!isspace(*endptr) *endptr != ',' *endptr != 0) {
+if (!qemu_isspace(*endptr) *endptr != ',' *endptr != 0) {
goto fail;
}
}
--
1.7.3.4
[Qemu-devel] [PATCH 3/4] strtosz(): Fix name confusion in use of modf()
From: Jes Sorensen jes.soren...@redhat.com
Signed-off-by: Jes Sorensen jes.soren...@redhat.com
---
cutils.c |4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/cutils.c b/cutils.c
index 78d35e2..369a016 100644
--- a/cutils.c
+++ b/cutils.c
@@ -304,8 +304,8 @@ int64_t strtosz_suffix(const char *nptr, char **end, const
char default_suffix)
if (isnan(val) || endptr == nptr || errno != 0) {
goto fail;
}
-integral = modf(val, fraction);
-if (integral != 0) {
+fraction = modf(val, integral);
+if (fraction != 0) {
mul_required = 1;
}
/*
--
1.7.3.4
[Qemu-devel] [PATCH 4/4] strtosz(): Use suffix macros in switch() statement
From: Jes Sorensen jes.soren...@redhat.com
Signed-off-by: Jes Sorensen jes.soren...@redhat.com
---
cutils.c | 10 +-
1 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/cutils.c b/cutils.c
index 369a016..8d562b2 100644
--- a/cutils.c
+++ b/cutils.c
@@ -324,26 +324,26 @@ int64_t strtosz_suffix(const char *nptr, char **end,
const char default_suffix)
}
}
switch (qemu_toupper(d)) {
-case 'B':
+case STRTOSZ_DEFSUFFIX_B:
mul = 1;
if (mul_required) {
goto fail;
}
break;
-case 'K':
+case STRTOSZ_DEFSUFFIX_KB:
mul = 1 10;
break;
case 0:
if (mul_required) {
goto fail;
}
-case 'M':
+case STRTOSZ_DEFSUFFIX_MB:
mul = 1ULL 20;
break;
-case 'G':
+case STRTOSZ_DEFSUFFIX_GB:
mul = 1ULL 30;
break;
-case 'T':
+case STRTOSZ_DEFSUFFIX_TB:
mul = 1ULL 40;
break;
default:
--
1.7.3.4
Re: [Qemu-devel] [PATCH 4/4] strtosz(): Use suffix macros in switch() statement
On 01/24/11 17:08, Markus Armbruster wrote:
jes.soren...@redhat.com writes:
From: Jes Sorensen jes.soren...@redhat.com
Signed-off-by: Jes Sorensen jes.soren...@redhat.com
---
cutils.c | 10 +-
1 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/cutils.c b/cutils.c
index 369a016..8d562b2 100644
--- a/cutils.c
+++ b/cutils.c
@@ -324,26 +324,26 @@ int64_t strtosz_suffix(const char *nptr, char **end,
const char default_suffix)
}
}
switch (qemu_toupper(d)) {
-case 'B':
+case STRTOSZ_DEFSUFFIX_B:
mul = 1;
if (mul_required) {
goto fail;
}
break;
-case 'K':
+case STRTOSZ_DEFSUFFIX_KB:
mul = 1 10;
break;
case 0:
if (mul_required) {
goto fail;
}
-case 'M':
+case STRTOSZ_DEFSUFFIX_MB:
mul = 1ULL 20;
break;
-case 'G':
+case STRTOSZ_DEFSUFFIX_GB:
mul = 1ULL 30;
break;
-case 'T':
+case STRTOSZ_DEFSUFFIX_TB:
mul = 1ULL 40;
break;
default:
Phony abstraction. And it leaks: code here assumes the
STRTOSZ_DEFSUFFIX_T* are all upper case.
qemu_toupper() - whats the problem?
Jes
Re: [Qemu-devel] [PATCH 1/4] strtosz(): use unsigned char and switch to qemu_isspace()
On 01/24/11 17:10, Markus Armbruster wrote:
jes.soren...@redhat.com writes:
From: Jes Sorensen jes.soren...@redhat.com
isspace() behavior is undefined for signed char.
Bug pointed out by Eric Blake, thanks!
Signed-off-by: Jes Sorensen jes.soren...@redhat.com
---
cutils.c |7 ---
1 files changed, 4 insertions(+), 3 deletions(-)
diff --git a/cutils.c b/cutils.c
index 4d2e27c..a067cf4 100644
--- a/cutils.c
+++ b/cutils.c
@@ -294,7 +294,8 @@ int fcntl_setfl(int fd, int flag)
int64_t strtosz_suffix(const char *nptr, char **end, const char
default_suffix)
{
int64_t retval = -1;
-char *endptr, c, d;
+char *endptr;
+unsigned char c, d;
int mul_required = 0;
double val, mul, integral, fraction;
I doubt this hunk is still needed.
It isn't strictly due to qemu_toupper() but it is prettier to match the
real behavior of pure toupper() anyway.
Jes
Re: [Qemu-devel] [PATCH 4/4] strtosz(): Use suffix macros in switch() statement
On 01/24/11 17:39, Markus Armbruster wrote: +case STRTOSZ_DEFSUFFIX_TB: mul = 1ULL 40; break; default: Phony abstraction. And it leaks: code here assumes the STRTOSZ_DEFSUFFIX_T* are all upper case. qemu_toupper() - whats the problem? If a STRTOSZ_DEFSUFFIX_T? expands to a lower case character, its case will not match any input. Right, so one has to be careful when adding new suffix constants. However given that we already have all the likely to be used ones for the near future, that isn't exactly a big issue. On the other hand forcing the use of the macros makes it less likely that someone specifies an unsupported constant by hitting 'y' instead of 't' or similar. Jes
[Qemu-devel] Re: [RFC][PATCH v6 07/23] virtagent: base server definitions
On 01/24/11 17:51, Michael Roth wrote: On 01/24/2011 04:16 AM, Jes Sorensen wrote: It's obviously contentious, and it is not core functionality. In order to get the patches adapted upstream it would easy the process to remove it and keep it as a separate patch. Fair enough, the proposed copyfile replacement would be suitable as well. My main concern is stripping away too much functionality for the initial merge, since guest-initiated shutdown is all we'd really have left lacking viewdmesg/viewfile. Would it be better to get copyfile in for the initial set of patches, or as a subsequent set? Having copyfile would be good in an initial release too - however we should probably review it in the light of Dan's suggestion of using libguestfs. I am working on freeze/thaw support which I hope to have ready within a couple of days. It would be nice to get in, in an early release as well. Cheers, Jes
[Qemu-devel] Re: [RFC][PATCH v6 03/23] Make qemu timers available for tools
On 01/21/11 18:26, Michael Roth wrote: On 01/21/2011 10:30 AM, Jes Sorensen wrote: On 01/17/11 14:14, Michael Roth wrote: diff --git a/qemu-ioh.c b/qemu-ioh.c index cc71470..001e7a2 100644 --- a/qemu-ioh.c +++ b/qemu-ioh.c @@ -22,7 +22,11 @@ * THE SOFTWARE. */ #include qemu-ioh.h +#include qemu-char.h #include qlist.h +#ifdef CONFIG_EVENTFD +#includesys/eventfd.h +#endif /* XXX: fd_read_poll should be suppressed, but an API change is necessary in the character devices to suppress fd_can_read(). */ @@ -113,3 +117,92 @@ void qemu_process_fd_handlers2(void *ioh_record_list, const fd_set *rfds, } } } + +#ifndef _WIN32 +void iothread_event_increment(int *io_thread_fd) Please split the WIN32 stuff into it's own file, similar to oslib-posix and oslib-win32.c etc. Will look into this, but qemu-ioh.c has common code too so we'd end up with qemu-ioh/qemu-ioh-posix/qemu-ioh-win2.c. We could alternatively have a #ifndef _WIN32 around functions in qemu-ioh.c that would be replaced by win32-specific ones from qemu-ioh-win32. No strong preference either way, but sometimes I find navigating across too many files more annoying that #ifdefs, and there's not a whole lot in these. No problem having the three files - it is far better than having #ifdefs. Having the #ifndef that is overloaded by a win32 specific file is bad, it will make it very confusing for anyone reading the code. Cheers, Jes
[Qemu-devel] Re: [RFC][PATCH v6 03/23] Make qemu timers available for tools
On 01/17/11 14:14, Michael Roth wrote: diff --git a/qemu-ioh.c b/qemu-ioh.c index cc71470..001e7a2 100644 --- a/qemu-ioh.c +++ b/qemu-ioh.c @@ -22,7 +22,11 @@ * THE SOFTWARE. */ #include qemu-ioh.h +#include qemu-char.h #include qlist.h +#ifdef CONFIG_EVENTFD +#include sys/eventfd.h +#endif /* XXX: fd_read_poll should be suppressed, but an API change is necessary in the character devices to suppress fd_can_read(). */ @@ -113,3 +117,92 @@ void qemu_process_fd_handlers2(void *ioh_record_list, const fd_set *rfds, } } } + +#ifndef _WIN32 +void iothread_event_increment(int *io_thread_fd) Please split the WIN32 stuff into it's own file, similar to oslib-posix and oslib-win32.c etc. diff --git a/qemu-ioh.h b/qemu-ioh.h index 7c6e833..2c714a9 100644 --- a/qemu-ioh.h +++ b/qemu-ioh.h @@ -31,4 +31,13 @@ void qemu_get_fdset2(void *ioh_record_list, int *nfds, fd_set *rfds, void qemu_process_fd_handlers2(void *ioh_record_list, const fd_set *rfds, const fd_set *wfds, const fd_set *xfds); + +#ifndef _WIN32 +void iothread_event_increment(int *io_thread_fd); +int iothread_event_init(int *io_thread_fd); +#else +int win32_event_init(HANDLE *qemu_event_handle); +void win32_event_increment(HANDLE *qemu_event_handle); +#endif Can you not do something slightly nicer that allows for those to be the same prototype for all users? Like define a event_handle_t? + +#ifndef _WIN32 +static int io_thread_fd = -1; Needs splitting into separate files too. diff --git a/qemu-tool.h b/qemu-tool.h new file mode 100644 index 000..fd693cf --- /dev/null +++ b/qemu-tool.h @@ -0,0 +1,26 @@ +#ifndef QEMU_TOOL_H +#define QEMU_TOOL_H + +#include qemu-common.h + +#ifdef CONFIG_EVENTFD +#include sys/eventfd.h +#endif + +typedef void VMStateDescription; +typedef int VMStateInfo; + +#ifndef _WIN32 +void qemu_event_increment(void); +int qemu_event_init(void); +#else +int qemu_event_init(void); +void qemu_event_increment(void); +#endif No matter how long I stare at those prototypes, I fail to see the difference between the win32 and the posix version :) Cheers, Jes
[Qemu-devel] Re: [RFC][PATCH v6 07/23] virtagent: base server definitions
diff --git a/virtagent-server.h b/virtagent-server.h new file mode 100644 index 000..9f68921 --- /dev/null +++ b/virtagent-server.h @@ -0,0 +1,34 @@ +/* + * virt-agent - host/guest RPC daemon functions + * + * Copyright IBM Corp. 2010 + * + * Authors: + * Michael Roth mdr...@linux.vnet.ibm.com + * + * This work is licensed under the terms of the GNU GPL, version 2 or later. + * See the COPYING file in the top-level directory. + * + */ + +#include xmlrpc-c/base.h +#include xmlrpc-c/server.h + +#define GUEST_AGENT_SERVICE_ID virtagent +#define GUEST_AGENT_PATH /tmp/virtagent-guest.sock +#define HOST_AGENT_SERVICE_ID virtagent-host +#define HOST_AGENT_PATH /tmp/virtagent-host.sock +#define VA_GETFILE_MAX 1 30 +#define VA_FILEBUF_LEN 16384 +#define VA_DMESG_LEN 16384 I really don't like these hard coded constants - you you have a command line interface allowing for the change of the sockets and file names? Otherwise you'll hit problems on the host side with concurrent runs of qemu. I really would like to see the dmesg stuff removed too for now as we discussed earlier. Cheers, Jes
[Qemu-devel] Re: [RFC][PATCH v6 08/23] virtagent: add va.getfile RPC
On 01/17/11 14:15, Michael Roth wrote:
Add RPC to retrieve a guest file. This interface is intended
for smaller reads like peeking at logs and /proc and such.
Signed-off-by: Michael Roth mdr...@linux.vnet.ibm.com
---
virtagent-server.c | 59
1 files changed, 59 insertions(+), 0 deletions(-)
diff --git a/virtagent-server.c b/virtagent-server.c
index c38a9e0..af4b940 100644
--- a/virtagent-server.c
+++ b/virtagent-server.c
@@ -62,12 +62,71 @@ out:
return ret;
}
+/* RPC functions common to guest/host daemons */
+
+/* va_getfile(): return file contents
+ * rpc return values:
+ * - base64-encoded file contents
+ */
+static xmlrpc_value *va_getfile(xmlrpc_env *env,
+xmlrpc_value *params,
+void *user_data)
+{
+const char *path;
+char *file_contents = NULL;
+char buf[VA_FILEBUF_LEN];
malloc()!
+int fd, ret, count = 0;
+xmlrpc_value *result = NULL;
+
+/* parse argument array */
+xmlrpc_decompose_value(env, params, (s), path);
+if (env-fault_occurred) {
+return NULL;
+}
+
+SLOG(va_getfile(), path:%s, path);
+
+fd = open(path, O_RDONLY);
+if (fd == -1) {
+LOG(open failed: %s, strerror(errno));
+xmlrpc_faultf(env, open failed: %s, strerror(errno));
+return NULL;
+}
+
+while ((ret = read(fd, buf, VA_FILEBUF_LEN)) 0) {
+file_contents = qemu_realloc(file_contents, count + VA_FILEBUF_LEN);
+memcpy(file_contents + count, buf, ret);
Sorry, I brought this up before. This realloc() stuff is a disaster
waiting to happen. Please remove it from the patch series, until you
have an implementation that copies over a page of the time.
+count += ret;
Cheers,
Jes
[Qemu-devel] Re: [RFC][PATCH v6 09/23] virtagent: add agent_viewfile qmp/hmp command
On 01/17/11 14:15, Michael Roth wrote: Utilize the getfile RPC to provide a means to view text files in the guest. Getfile can handle binary files as well but we don't advertise that here due to the special handling requiring to store it and provide it back to the user (base64 encoding it for instance). Hence the otherwise confusing viewfile as opposed to getfile. I am really against this in any shape or form. Please do a copy and view on the host. Cheers, Jes
Re: [Qemu-devel] [PATCH 2/2] strtosz(): Use suffix macros in switch() statement
On 01/18/11 21:39, Anthony Liguori wrote: On 01/18/2011 02:36 PM, Jes Sorensen wrote: On 01/18/11 21:30, Anthony Liguori wrote: On 01/18/2011 10:53 AM, Eric Blake wrote: And it does, via the toupper() added earlier in the series (and which has separately been pointed out that using qemu_toupper() might be nicer). Ok. Just taking the different case labels would be nicer IMHO. The old code did that, but I was suggested to do it this way, which I think is cleaner too. Fewer lines of code are easier to read. toupper() is based on locale so it's not consistent. If you can show me an actually used locale where the toupper() on k/m/g/t doesn't result in K/M/G/T then I guess there's a case. Otherwise I don't really see this being a real point. I think we are hitting the point where it's about who's taste is better, and not about the actual code in this discussion. One point in favor of the patch is this: Without the patch: jes@red-feather qemu]$ size cutils.o textdata bss dec hex filename 4212 0 042121074 cutils.o With patch: [jes@red-feather qemu]$ size cutils.o textdata bss dec hex filename 4196 0 041961064 cutils.o IMHO it makes the code easier to read, but beyond that there isn't much. If people are strongly against it, I'll just drop the patch. It's not worth our time arguing over this level of detail. Otherwise I'd like to see it applied. Cheers, Jes
Re: [Qemu-devel] [PATCH 2/2] strtosz(): Use suffix macros in switch() statement
On 01/18/11 10:20, Markus Armbruster wrote:
diff --git a/cutils.c b/cutils.c
index 328738c..f2c8bbd 100644
--- a/cutils.c
+++ b/cutils.c
@@ -324,26 +324,26 @@ ssize_t strtosz_suffix(const char *nptr, char **end,
const char default_suffix)
}
}
switch (toupper(d)) {
-case 'B':
+case STRTOSZ_DEFSUFFIX_B:
mul = 1;
if (mul_required) {
goto fail;
}
break;
-case 'K':
+case STRTOSZ_DEFSUFFIX_KB:
mul = 1 10;
break;
case 0:
if (mul_required) {
goto fail;
}
-case 'M':
+case STRTOSZ_DEFSUFFIX_MB:
mul = 1ULL 20;
break;
-case 'G':
+case STRTOSZ_DEFSUFFIX_GB:
mul = 1ULL 30;
break;
-case 'T':
+case STRTOSZ_DEFSUFFIX_TB:
mul = 1ULL 40;
break;
default:
And this improves what? Certainly not clarity.
In my opinion, the STRTOSZ_DEFSUFFIX_TB are useless chaff. Chacun à son
goût.
It cuts out lines of code, which is good, and using the macros means the
user is less likely to make a type and use a wrong character.
It's a taste issue though, I agree!
Cheers,
Jes
Re: [Qemu-devel] [PATCH 2/2] strtosz(): Use suffix macros in switch() statement
On 01/18/11 17:50, Anthony Liguori wrote:
On 01/18/2011 03:20 AM, Markus Armbruster wrote:
jes.soren...@redhat.com writes:
From: Jes Sorensenjes.soren...@redhat.com
Signed-off-by: Jes Sorensenjes.soren...@redhat.com
---
cutils.c | 10 +-
1 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/cutils.c b/cutils.c
index 328738c..f2c8bbd 100644
--- a/cutils.c
+++ b/cutils.c
@@ -324,26 +324,26 @@ ssize_t strtosz_suffix(const char *nptr, char
**end, const char default_suffix)
}
}
switch (toupper(d)) {
^^^
And this improves what? Certainly not clarity.
In my opinion, the STRTOSZ_DEFSUFFIX_TB are useless chaff. Chacun à son
goût.
Yeah, I have to agree. I'm not of the literals are evil camp.
BTW, a useful change would be to accept both upper and lower case letters.
It already supports both, it's handle in the toupper() call.
Cheers,
Jes
Re: [Qemu-devel] [PATCH 2/2] strtosz(): Use suffix macros in switch() statement
On 01/18/11 21:30, Anthony Liguori wrote:
On 01/18/2011 10:53 AM, Eric Blake wrote:
On 01/18/2011 09:50 AM, Anthony Liguori wrote:
@@ -324,26 +324,26 @@ ssize_t strtosz_suffix(const char *nptr, char
**end, const char default_suffix)
}
}
switch (toupper(d)) {
BTW, a useful change would be to accept both upper and lower case
letters.
And it does, via the toupper() added earlier in the series (and which
has separately been pointed out that using qemu_toupper() might be
nicer).
Ok. Just taking the different case labels would be nicer IMHO.
The old code did that, but I was suggested to do it this way, which I
think is cleaner too. Fewer lines of code are easier to read.
Cheers,
Jes
[Qemu-devel] [PATCH 1/2] strtosz() use unsigned char as isspace() is not defined for signed char
From: Jes Sorensen jes.soren...@redhat.com
Bug pointed out by Erik Blake, thanks!
Signed-off-by: Jes Sorensen jes.soren...@redhat.com
---
cutils.c |3 ++-
1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/cutils.c b/cutils.c
index 7984bc1..f97ef39 100644
--- a/cutils.c
+++ b/cutils.c
@@ -294,7 +294,8 @@ int fcntl_setfl(int fd, int flag)
ssize_t strtosz_suffix(const char *nptr, char **end, const char default_suffix)
{
ssize_t retval = -1;
-char *endptr, c, d;
+char *endptr;
+unsigned char c, d;
int mul_required = 0;
double val, mul, integral, fraction;
--
1.7.3.4
[Qemu-devel] [PATCH 2/2] strtosz() use toupper() to simply switch statement
From: Jes Sorensen jes.soren...@redhat.com
Signed-off-by: Jes Sorensen jes.soren...@redhat.com
---
cutils.c |7 +--
1 files changed, 1 insertions(+), 6 deletions(-)
diff --git a/cutils.c b/cutils.c
index f97ef39..f7aa90c 100644
--- a/cutils.c
+++ b/cutils.c
@@ -323,16 +323,14 @@ ssize_t strtosz_suffix(const char *nptr, char **end,
const char default_suffix)
d = c;
}
}
-switch (d) {
+switch (toupper(d)) {
case 'B':
-case 'b':
mul = 1;
if (mul_required) {
goto fail;
}
break;
case 'K':
-case 'k':
mul = 1 10;
break;
case 0:
@@ -340,15 +338,12 @@ ssize_t strtosz_suffix(const char *nptr, char **end,
const char default_suffix)
goto fail;
}
case 'M':
-case 'm':
mul = 1ULL 20;
break;
case 'G':
-case 'g':
mul = 1ULL 30;
break;
case 'T':
-case 't':
mul = 1ULL 40;
break;
default:
--
1.7.3.4
[Qemu-devel] [PATCH v2 0/2] strtosz() cleanup
From: Jes Sorensen jes.soren...@redhat.com Fix issue with signed char and isspace() as well as clean up switch statement using toupper(). V2 of the patch fixes some types in the commit messages and spells Eric correctly. My apologies for getting it wrong. Jes Sorensen (2): strtosz() use unsigned char as isspace() is not defined for signed char strtosz() use toupper() to simplify switch statement cutils.c | 10 +++--- 1 files changed, 3 insertions(+), 7 deletions(-) -- 1.7.3.4
[Qemu-devel] [PATCH 2/2] strtosz() use toupper() to simplify switch statement
From: Jes Sorensen jes.soren...@redhat.com
Signed-off-by: Jes Sorensen jes.soren...@redhat.com
---
cutils.c |7 +--
1 files changed, 1 insertions(+), 6 deletions(-)
diff --git a/cutils.c b/cutils.c
index f97ef39..f7aa90c 100644
--- a/cutils.c
+++ b/cutils.c
@@ -323,16 +323,14 @@ ssize_t strtosz_suffix(const char *nptr, char **end,
const char default_suffix)
d = c;
}
}
-switch (d) {
+switch (toupper(d)) {
case 'B':
-case 'b':
mul = 1;
if (mul_required) {
goto fail;
}
break;
case 'K':
-case 'k':
mul = 1 10;
break;
case 0:
@@ -340,15 +338,12 @@ ssize_t strtosz_suffix(const char *nptr, char **end,
const char default_suffix)
goto fail;
}
case 'M':
-case 'm':
mul = 1ULL 20;
break;
case 'G':
-case 'g':
mul = 1ULL 30;
break;
case 'T':
-case 't':
mul = 1ULL 40;
break;
default:
--
1.7.3.4
[Qemu-devel] [PATCH 1/2] strtosz() use unsigned char as isspace() is not defined for signed char
From: Jes Sorensen jes.soren...@redhat.com
Bug pointed out by Eric Blake, thanks!
Signed-off-by: Jes Sorensen jes.soren...@redhat.com
---
cutils.c |3 ++-
1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/cutils.c b/cutils.c
index 7984bc1..f97ef39 100644
--- a/cutils.c
+++ b/cutils.c
@@ -294,7 +294,8 @@ int fcntl_setfl(int fd, int flag)
ssize_t strtosz_suffix(const char *nptr, char **end, const char default_suffix)
{
ssize_t retval = -1;
-char *endptr, c, d;
+char *endptr;
+unsigned char c, d;
int mul_required = 0;
double val, mul, integral, fraction;
--
1.7.3.4
[Qemu-devel] [PATCH 1/2] strtosz(): Fix name confusion in use of modf()
From: Jes Sorensen jes.soren...@redhat.com
Signed-off-by: Jes Sorensen jes.soren...@redhat.com
---
cutils.c |4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/cutils.c b/cutils.c
index f7aa90c..328738c 100644
--- a/cutils.c
+++ b/cutils.c
@@ -304,8 +304,8 @@ ssize_t strtosz_suffix(const char *nptr, char **end, const
char default_suffix)
if (isnan(val) || endptr == nptr || errno != 0) {
goto fail;
}
-integral = modf(val, fraction);
-if (integral != 0) {
+fraction = modf(val, integral);
+if (fraction != 0) {
mul_required = 1;
}
/*
--
1.7.3.4
[Qemu-devel] [PATCH 0/2] strtosz() more cleanups
From: Jes Sorensen jes.soren...@redhat.com Another two cleanups to strtosz() suggested by Alex Williamson. I had reversed the use of the names of the return arguments to modf(), and use the STRTOSZ_DESUFFIX_ macros in the switch() statement. Cheers, Jes Jes Sorensen (2): strtosz(): Fix name confusion in use of modf() strtosz(): Use suffix macros in switch() statement cutils.c | 14 +++--- 1 files changed, 7 insertions(+), 7 deletions(-) -- 1.7.3.4
[Qemu-devel] [PATCH 2/2] strtosz(): Use suffix macros in switch() statement
From: Jes Sorensen jes.soren...@redhat.com
Signed-off-by: Jes Sorensen jes.soren...@redhat.com
---
cutils.c | 10 +-
1 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/cutils.c b/cutils.c
index 328738c..f2c8bbd 100644
--- a/cutils.c
+++ b/cutils.c
@@ -324,26 +324,26 @@ ssize_t strtosz_suffix(const char *nptr, char **end,
const char default_suffix)
}
}
switch (toupper(d)) {
-case 'B':
+case STRTOSZ_DEFSUFFIX_B:
mul = 1;
if (mul_required) {
goto fail;
}
break;
-case 'K':
+case STRTOSZ_DEFSUFFIX_KB:
mul = 1 10;
break;
case 0:
if (mul_required) {
goto fail;
}
-case 'M':
+case STRTOSZ_DEFSUFFIX_MB:
mul = 1ULL 20;
break;
-case 'G':
+case STRTOSZ_DEFSUFFIX_GB:
mul = 1ULL 30;
break;
-case 'T':
+case STRTOSZ_DEFSUFFIX_TB:
mul = 1ULL 40;
break;
default:
--
1.7.3.4
