[Qemu-devel] Re: [PATCH] cutils: Use int64_t instead of ssize_t for strtosz()
On 01/10/11 11:29, Stefan Hajnoczi wrote: The strtosz() function parses byte count strings and converts K, M, G units. The ssize_t type is not appropriate because block devices need 64-bit range even on 32-bit hosts. Switch from ssize_t to int64_t. Hmmm I think this is identical to the patch I posted last week :) Cheers, Jes
Re: [Qemu-devel] Re: KVM call agenda for Jan 11
On 01/10/11 12:59, Juan Quintela wrote: Juan Quintela quint...@redhat.com wrote: Now sent it to the right kvm list. Sorry for the second sent. Please send any agenda items you are interested in covering. - KVM Forum 2011 (Jes). Just to add a bit more background. Last year we discussed the issue of whether to aim for a KVM Forum in the same style as we had in 2010, or whether to try to aim for a broader multi-track Virtualization conference that covers the whole stack. Linux Foundation is happy to help host such an event, but they are asking for what our plans are. I posted a mock-proposal for tracks here: http://www.linux-kvm.org/page/KVM_Forum_2011 Cheers, Jes
[Qemu-devel] [PATCH] do_snapshot_blkdev() error on missing snapshot_file argument
From: Jes Sorensen jes.soren...@redhat.com
Current code does not support snapshot internally to the running
image. Error in case no snapshot_file is specified.
Signed-off-by: Jes Sorensen jes.soren...@redhat.com
---
blockdev.c |6 ++
1 files changed, 6 insertions(+), 0 deletions(-)
diff --git a/blockdev.c b/blockdev.c
index d7add36..662f7a9 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -526,6 +526,12 @@ int do_snapshot_blkdev(Monitor *mon, const QDict *qdict,
QObject **ret_data)
int ret = 0;
int flags;
+if (!filename) {
+qerror_report(QERR_MISSING_PARAMETER, snapshot_file);
+ret = -1;
+goto out;
+}
+
bs = bdrv_find(device);
if (!bs) {
qerror_report(QERR_DEVICE_NOT_FOUND, device);
--
1.7.3.4
Re: [Qemu-devel] Re: Usefulness of the bug tracker
On 01/06/11 16:11, Michael S. Tsirkin wrote: On Thu, Jan 06, 2011 at 10:28:46AM +, Stefan Hajnoczi wrote: The real problem is that we're collecting bugs but not effectively investigating and fixing them. Can Launchpad send out automatic bug summary emails once every week/two weeks/month? Launchpad is at least part of the problem. Users find it without going through the wiki and file Ubuntu/Debian specific bugs there. It is definitely a problem that a large part of the people who submit bugs in Launchpad take it for granted that it is for Ubuntu/Debian. I would quite welcome a tracker on qemu.org I am still trying to get rid of all the bugs in the sourceforge tracker, and move over the ones that we need to keep, but I could use some help. Cheers, Jes
[Qemu-devel] [PATCH] Make strtosz() return int64_t instead of ssize_t
From: Jes Sorensen jes.soren...@redhat.com
strtosz() needs to return a 64 bit type even on 32 bit
architectures. Otherwise qemu-img will fail to create disk
images = 2GB
Signed-off-by: Jes Sorensen jes.soren...@redhat.com
---
cutils.c |8
monitor.c |2 +-
qemu-common.h |4 ++--
qemu-img.c|2 +-
vl.c |4 ++--
5 files changed, 10 insertions(+), 10 deletions(-)
diff --git a/cutils.c b/cutils.c
index 7984bc1..4d2e27c 100644
--- a/cutils.c
+++ b/cutils.c
@@ -291,9 +291,9 @@ int fcntl_setfl(int fd, int flag)
* value must be terminated by whitespace, ',' or '\0'. Return -1 on
* error.
*/
-ssize_t strtosz_suffix(const char *nptr, char **end, const char default_suffix)
+int64_t strtosz_suffix(const char *nptr, char **end, const char default_suffix)
{
-ssize_t retval = -1;
+int64_t retval = -1;
char *endptr, c, d;
int mul_required = 0;
double val, mul, integral, fraction;
@@ -365,7 +365,7 @@ ssize_t strtosz_suffix(const char *nptr, char **end, const
char default_suffix)
goto fail;
}
}
-if ((val * mul = ~(size_t)0) || val 0) {
+if ((val * mul = INT64_MAX) || val 0) {
goto fail;
}
retval = val * mul;
@@ -378,7 +378,7 @@ fail:
return retval;
}
-ssize_t strtosz(const char *nptr, char **end)
+int64_t strtosz(const char *nptr, char **end)
{
return strtosz_suffix(nptr, end, STRTOSZ_DEFSUFFIX_MB);
}
diff --git a/monitor.c b/monitor.c
index f258000..fcdae15 100644
--- a/monitor.c
+++ b/monitor.c
@@ -4162,7 +4162,7 @@ static const mon_cmd_t *monitor_parse_command(Monitor
*mon,
break;
case 'o':
{
-ssize_t val;
+int64_t val;
char *end;
while (qemu_isspace(*p)) {
diff --git a/qemu-common.h b/qemu-common.h
index 63d9943..cce6e61 100644
--- a/qemu-common.h
+++ b/qemu-common.h
@@ -158,8 +158,8 @@ int fcntl_setfl(int fd, int flag);
#define STRTOSZ_DEFSUFFIX_MB 'M'
#define STRTOSZ_DEFSUFFIX_KB 'K'
#define STRTOSZ_DEFSUFFIX_B'B'
-ssize_t strtosz(const char *nptr, char **end);
-ssize_t strtosz_suffix(const char *nptr, char **end, const char
default_suffix);
+int64_t strtosz(const char *nptr, char **end);
+int64_t strtosz_suffix(const char *nptr, char **end, const char
default_suffix);
/* path.c */
void init_paths(const char *prefix);
diff --git a/qemu-img.c b/qemu-img.c
index afd9ed2..6af2a4c 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -320,7 +320,7 @@ static int img_create(int argc, char **argv)
/* Get image size, if specified */
if (optind argc) {
-ssize_t sval;
+int64_t sval;
sval = strtosz_suffix(argv[optind++], NULL, STRTOSZ_DEFSUFFIX_B);
if (sval 0) {
error_report(Invalid image size specified! You may use k, M, G or
diff --git a/vl.c b/vl.c
index 78fcef1..93425f4 100644
--- a/vl.c
+++ b/vl.c
@@ -804,7 +804,7 @@ static void numa_add(const char *optarg)
if (get_param_value(option, 128, mem, optarg) == 0) {
node_mem[nodenr] = 0;
} else {
-ssize_t sval;
+int64_t sval;
sval = strtosz(option, NULL);
if (sval 0) {
fprintf(stderr, qemu: invalid numa mem size: %s\n, optarg);
@@ -2245,7 +2245,7 @@ int main(int argc, char **argv, char **envp)
exit(0);
break;
case QEMU_OPTION_m: {
-ssize_t value;
+int64_t value;
value = strtosz(optarg, NULL);
if (value 0) {
--
1.7.3.4
[Qemu-devel] Re: [PATCH] Make strtosz() return int64_t instead of ssize_t
On 01/05/11 13:34, Michael S. Tsirkin wrote: On Wed, Jan 05, 2011 at 11:41:02AM +0100, jes.soren...@redhat.com wrote: From: Jes Sorensen jes.soren...@redhat.com strtosz() needs to return a 64 bit type even on 32 bit architectures. Otherwise qemu-img will fail to create disk images = 2GB Signed-off-by: Jes Sorensen jes.soren...@redhat.com Nothing wrong with this patch, but should the function be renamed to strtos64 then? I don't think that adds any value to be honest. The problem with the old interface was that the return type differed depending on whether it was compiled on 32 vs 64 bit systems. Cheers, Jes
[Qemu-devel] Re: [PATCH] Make strtosz() return int64_t instead of ssize_t
On 01/05/11 14:46, Anthony Liguori wrote: On 01/05/2011 04:41 AM, jes.soren...@redhat.com wrote: From: Jes Sorensenjes.soren...@redhat.com strtosz() needs to return a 64 bit type even on 32 bit architectures. Otherwise qemu-img will fail to create disk images= 2GB Signed-off-by: Jes Sorensenjes.soren...@redhat.com off_t would be the proper type to use. I discussed this with Markus, and we both agree that it isn't. Two reasons, off_t is for filesystem offsets, not random sizes. Second, off_t doesn't have an unsigned type or a max to compare against. Using int64_t is cleaner and safer. Cheers, Jes
[Qemu-devel] Re: [PATCH] Make strtosz() return int64_t instead of ssize_t
On 01/05/11 19:29, Anthony Liguori wrote: I wouldn't make such bold claims but I'll concede that one is not significantly better than the other and won't object to int64_t if you feel strongly. The more I think of it, the more I come to the conclusion that int64_t is the best solution. Since we can in theory have a system where we only allow 32 bit file system offsets, but have 4GB of memory, int64_t does it best - it is more generic. Cheers, Jes
Re: [Qemu-devel] KVM call agenda for Jan 4
On 01/03/11 11:57, Juan Quintela wrote: Please send any agenda items you are interested in covering. thanks, Juan. Do we have anything for the agenda yet? Jes
Re: [Qemu-devel] KVM call agenda for Jan 4
On 01/04/11 15:33, Anthony Liguori wrote: On 01/04/2011 08:31 AM, Jes Sorensen wrote: On 01/03/11 11:57, Juan Quintela wrote: Please send any agenda items you are interested in covering. thanks, Juan. Do we have anything for the agenda yet? I could use the extra hour to catch up from the Holiday (as I assume a lot of folks could). I'd suggest cancelling. Only if you promise to apply a lot of pending patches during that hour :) Cheers, Jes
[Qemu-devel] Re: [PATCH 1/2] block/qcow2.c: rename qcow_ functions to qcow2_
On 12/17/10 15:20, Kevin Wolf wrote:
offset = start_offset;
while (offset end_offset) {
@@ -88,13 +88,13 @@ static int qcow_read_extensions(BlockDriverState *bs,
uint64_t start_offset,
#ifdef DEBUG_EXT
/* Sanity check */
if (offset s-cluster_size)
-printf(qcow_handle_extension: suspicious offset %lu\n,
offset);
+printf(qcow_read_extension: suspicious offset %lu\n, offset);
It's now qcow2_read_extensions
Fixed
@@ -313,7 +313,7 @@ static int qcow_is_allocated(BlockDriverState *bs,
int64_t sector_num,
/* handle reading after the end of the backing file */
int qcow2_backing_read1(BlockDriverState *bs, QEMUIOVector *qiov,
- int64_t sector_num, int nb_sectors)
+int64_t sector_num, int nb_sectors)
This isn't related to renaming functions. Please don't include pure
formatting changes, all they do is making git blame work worse.
No it makes the formatting consistent with the rest of the functions in
the file. I can leave it out, but then we just have more ugliness in the
file.
@@ -399,10 +399,11 @@ static void qcow_aio_read_cb(void *opaque, int ret)
} else {
if (s-crypt_method) {
qcow2_encrypt_sectors(s, acb-sector_num, acb-cluster_data,
-acb-cluster_data, acb-cur_nr_sectors, 0,
s-aes_decrypt_key);
+ acb-cluster_data, acb-cur_nr_sectors,
+ 0, s-aes_decrypt_key);
Same here, plus the old version wasn't obviously indented wrong, but
just not according to your personal style.
Sorry it's broken formatting. But sure, I'll put it back to being
unreadable.
The following changes include more lines that need not be changed for
the rename and just change the coding style (even though CODING_STYLE
doesn't make a statement on this, so the old version isn't wrong).
Please leave them out.
Actually that is in the patch, I did a pure search replace, no
formatting. But I've fixed it.
Jes
[Qemu-devel] Re: [PATCH 2/2] Add proper -errno error return values to qcow2_open()
On 12/17/10 15:27, Kevin Wolf wrote:
Am 16.12.2010 17:05, schrieb jes.soren...@redhat.com:
From: Jes Sorensen jes.soren...@redhat.com
In addition this adds missing braces to the function to be consistent
with the coding style.
Signed-off-by: Jes Sorensen jes.soren...@redhat.com
---
block/qcow2.c | 61
1 files changed, 43 insertions(+), 18 deletions(-)
diff --git a/block/qcow2.c b/block/qcow2.c
index d7fd167..b4a9e5e 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -140,12 +140,14 @@ static int qcow2_read_extensions(BlockDriverState *bs,
uint64_t start_offset,
static int qcow2_open(BlockDriverState *bs, int flags)
{
BDRVQcowState *s = bs-opaque;
-int len, i;
+int len, i, ret = 0;
QCowHeader header;
uint64_t ext_end;
-if (bdrv_pread(bs-file, 0, header, sizeof(header)) != sizeof(header))
+if (bdrv_pread(bs-file, 0, header, sizeof(header)) != sizeof(header))
{
+ret = -EIO;
goto fail;
+}
ret = bdrv_pread(...);
if (ret 0) {
goto fail;
}
Hmmm I must have confused something and looked at a wrong pread function
where it just returned -1 on error. I'll fix it.
Thanks,
Jes
[Qemu-devel] [PATCH v2 0/2] qcow2 cleanups
From: Jes Sorensen jes.soren...@redhat.com Hi, These two patches tries to clean up the qcow2 code a little. First makes the function names consistent, ie. we shouldn't have qcow_ functions in the qcow2 code. Second tries to add proper errno return values to qcow2_open(). New in v2: Fix the bdrv_pread() handling as pointed out by Kevin. Fix error messages, and restore a couple of cases to their unreadable formatting to avoid formatting changes not directly related to the qcow_-qcow2_ rename. Jes Jes Sorensen (2): block/qcow2.c: rename qcow_ functions to qcow2_ Add proper -errno error return values to qcow2_open() block/qcow2-cluster.c |6 +- block/qcow2-snapshot.c |6 +- block/qcow2.c | 248 +++- 3 files changed, 145 insertions(+), 115 deletions(-) -- 1.7.3.3
[Qemu-devel] [PATCH 1/2] block/qcow2.c: rename qcow_ functions to qcow2_
From: Jes Sorensen jes.soren...@redhat.com
It doesn't really make sense for functions in qcow2.c to be named
qcow_ so convert the names to match correctly.
Signed-off-by: Jes Sorensen jes.soren...@redhat.com
---
block/qcow2-cluster.c |6 +-
block/qcow2-snapshot.c |6 +-
block/qcow2.c | 190 +---
3 files changed, 104 insertions(+), 98 deletions(-)
diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index b040208..6928c63 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -352,8 +352,8 @@ void qcow2_encrypt_sectors(BDRVQcowState *s, int64_t
sector_num,
}
-static int qcow_read(BlockDriverState *bs, int64_t sector_num,
- uint8_t *buf, int nb_sectors)
+static int qcow2_read(BlockDriverState *bs, int64_t sector_num,
+ uint8_t *buf, int nb_sectors)
{
BDRVQcowState *s = bs-opaque;
int ret, index_in_cluster, n, n1;
@@ -419,7 +419,7 @@ static int copy_sectors(BlockDriverState *bs, uint64_t
start_sect,
if (n = 0)
return 0;
BLKDBG_EVENT(bs-file, BLKDBG_COW_READ);
-ret = qcow_read(bs, start_sect + n_start, s-cluster_data, n);
+ret = qcow2_read(bs, start_sect + n_start, s-cluster_data, n);
if (ret 0)
return ret;
if (s-crypt_method) {
diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
index aacf357..74823a5 100644
--- a/block/qcow2-snapshot.c
+++ b/block/qcow2-snapshot.c
@@ -116,7 +116,7 @@ int qcow2_read_snapshots(BlockDriverState *bs)
}
/* add at the end of the file a new list of snapshots */
-static int qcow_write_snapshots(BlockDriverState *bs)
+static int qcow2_write_snapshots(BlockDriverState *bs)
{
BDRVQcowState *s = bs-opaque;
QCowSnapshot *sn;
@@ -300,7 +300,7 @@ int qcow2_snapshot_create(BlockDriverState *bs,
QEMUSnapshotInfo *sn_info)
s-snapshots = snapshots1;
s-snapshots[s-nb_snapshots++] = *sn;
-if (qcow_write_snapshots(bs) 0)
+if (qcow2_write_snapshots(bs) 0)
goto fail;
#ifdef DEBUG_ALLOC
qcow2_check_refcounts(bs);
@@ -378,7 +378,7 @@ int qcow2_snapshot_delete(BlockDriverState *bs, const char
*snapshot_id)
qemu_free(sn-name);
memmove(sn, sn + 1, (s-nb_snapshots - snapshot_index - 1) * sizeof(*sn));
s-nb_snapshots--;
-ret = qcow_write_snapshots(bs);
+ret = qcow2_write_snapshots(bs);
if (ret 0) {
/* XXX: restore snapshot if error ? */
return ret;
diff --git a/block/qcow2.c b/block/qcow2.c
index 537c479..4b41190 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -50,10 +50,10 @@ typedef struct {
uint32_t magic;
uint32_t len;
} QCowExtension;
-#define QCOW_EXT_MAGIC_END 0
-#define QCOW_EXT_MAGIC_BACKING_FORMAT 0xE2792ACA
+#define QCOW2_EXT_MAGIC_END 0
+#define QCOW2_EXT_MAGIC_BACKING_FORMAT 0xE2792ACA
-static int qcow_probe(const uint8_t *buf, int buf_size, const char *filename)
+static int qcow2_probe(const uint8_t *buf, int buf_size, const char *filename)
{
const QCowHeader *cow_header = (const void *)buf;
@@ -73,14 +73,14 @@ static int qcow_probe(const uint8_t *buf, int buf_size,
const char *filename)
* unknown magic is skipped (future extension this version knows nothing about)
* return 0 upon success, non-0 otherwise
*/
-static int qcow_read_extensions(BlockDriverState *bs, uint64_t start_offset,
-uint64_t end_offset)
+static int qcow2_read_extensions(BlockDriverState *bs, uint64_t start_offset,
+ uint64_t end_offset)
{
QCowExtension ext;
uint64_t offset;
#ifdef DEBUG_EXT
-printf(qcow_read_extensions: start=%ld end=%ld\n, start_offset,
end_offset);
+printf(qcow2_read_extensions: start=%ld end=%ld\n, start_offset,
end_offset);
#endif
offset = start_offset;
while (offset end_offset) {
@@ -88,13 +88,13 @@ static int qcow_read_extensions(BlockDriverState *bs,
uint64_t start_offset,
#ifdef DEBUG_EXT
/* Sanity check */
if (offset s-cluster_size)
-printf(qcow_handle_extension: suspicious offset %lu\n, offset);
+printf(qcow2_read_extension: suspicious offset %lu\n, offset);
printf(attemting to read extended header in offset %lu\n, offset);
#endif
if (bdrv_pread(bs-file, offset, ext, sizeof(ext)) != sizeof(ext)) {
-fprintf(stderr, qcow_handle_extension: ERROR:
+fprintf(stderr, qcow2_read_extension: ERROR:
pread fail from offset % PRIu64 \n,
offset);
return 1;
@@ -106,10 +106,10 @@ static int qcow_read_extensions(BlockDriverState *bs,
uint64_t start_offset,
printf(ext.magic = 0x%x\n, ext.magic);
#endif
switch (ext.magic) {
-case QCOW_EXT_MAGIC_END:
+case QCOW2_EXT_MAGIC_END:
return 0;
-case QCOW_EXT_MAGIC_BACKING_FORMAT:
+case QCOW2_EXT_MAGIC_BACKING_FORMAT
[Qemu-devel] [PATCH 2/2] Add proper -errno error return values to qcow2_open()
From: Jes Sorensen jes.soren...@redhat.com
In addition this adds missing braces to the function to be consistent
with the coding style.
Signed-off-by: Jes Sorensen jes.soren...@redhat.com
---
block/qcow2.c | 60 +++-
1 files changed, 42 insertions(+), 18 deletions(-)
diff --git a/block/qcow2.c b/block/qcow2.c
index 4b41190..b6b094c 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -140,12 +140,14 @@ static int qcow2_read_extensions(BlockDriverState *bs,
uint64_t start_offset,
static int qcow2_open(BlockDriverState *bs, int flags)
{
BDRVQcowState *s = bs-opaque;
-int len, i;
+int len, i, ret = 0;
QCowHeader header;
uint64_t ext_end;
-if (bdrv_pread(bs-file, 0, header, sizeof(header)) != sizeof(header))
+ret = bdrv_pread(bs-file, 0, header, sizeof(header));
+if (ret 0) {
goto fail;
+}
be32_to_cpus(header.magic);
be32_to_cpus(header.version);
be64_to_cpus(header.backing_file_offset);
@@ -160,16 +162,23 @@ static int qcow2_open(BlockDriverState *bs, int flags)
be64_to_cpus(header.snapshots_offset);
be32_to_cpus(header.nb_snapshots);
-if (header.magic != QCOW_MAGIC || header.version != QCOW_VERSION)
+if (header.magic != QCOW_MAGIC || header.version != QCOW_VERSION) {
+ret = -EINVAL;
goto fail;
+}
if (header.cluster_bits MIN_CLUSTER_BITS ||
-header.cluster_bits MAX_CLUSTER_BITS)
+header.cluster_bits MAX_CLUSTER_BITS) {
+ret = -EINVAL;
goto fail;
-if (header.crypt_method QCOW_CRYPT_AES)
+}
+if (header.crypt_method QCOW_CRYPT_AES) {
+ret = -EINVAL;
goto fail;
+}
s-crypt_method_header = header.crypt_method;
-if (s-crypt_method_header)
+if (s-crypt_method_header) {
bs-encrypted = 1;
+}
s-cluster_bits = header.cluster_bits;
s-cluster_size = 1 s-cluster_bits;
s-cluster_sectors = 1 (s-cluster_bits - 9);
@@ -191,15 +200,19 @@ static int qcow2_open(BlockDriverState *bs, int flags)
s-l1_vm_state_index = size_to_l1(s, header.size);
/* the L1 table must contain at least enough entries to put
header.size bytes */
-if (s-l1_size s-l1_vm_state_index)
+if (s-l1_size s-l1_vm_state_index) {
+ret = -EINVAL;
goto fail;
+}
s-l1_table_offset = header.l1_table_offset;
if (s-l1_size 0) {
s-l1_table = qemu_mallocz(
align_offset(s-l1_size * sizeof(uint64_t), 512));
-if (bdrv_pread(bs-file, s-l1_table_offset, s-l1_table, s-l1_size *
sizeof(uint64_t)) !=
-s-l1_size * sizeof(uint64_t))
+ret = bdrv_pread(bs-file, s-l1_table_offset, s-l1_table,
+ s-l1_size * sizeof(uint64_t));
+if (ret 0) {
goto fail;
+}
for(i = 0;i s-l1_size; i++) {
be64_to_cpus(s-l1_table[i]);
}
@@ -212,35 +225,46 @@ static int qcow2_open(BlockDriverState *bs, int flags)
+ 512);
s-cluster_cache_offset = -1;
-if (qcow2_refcount_init(bs) 0)
+ret = qcow2_refcount_init(bs);
+if (ret != 0) {
goto fail;
+}
QLIST_INIT(s-cluster_allocs);
/* read qcow2 extensions */
-if (header.backing_file_offset)
+if (header.backing_file_offset) {
ext_end = header.backing_file_offset;
-else
+} else {
ext_end = s-cluster_size;
-if (qcow2_read_extensions(bs, sizeof(header), ext_end))
+}
+if (qcow2_read_extensions(bs, sizeof(header), ext_end)) {
+ret = -EINVAL;
goto fail;
+}
/* read the backing file name */
if (header.backing_file_offset != 0) {
len = header.backing_file_size;
-if (len 1023)
+if (len 1023) {
len = 1023;
-if (bdrv_pread(bs-file, header.backing_file_offset, bs-backing_file,
len) != len)
+}
+ret = bdrv_pread(bs-file, header.backing_file_offset,
+ bs-backing_file, len);
+if (ret 0) {
goto fail;
+}
bs-backing_file[len] = '\0';
}
-if (qcow2_read_snapshots(bs) 0)
+if (qcow2_read_snapshots(bs) 0) {
+ret = -EINVAL;
goto fail;
+}
#ifdef DEBUG_ALLOC
qcow2_check_refcounts(bs);
#endif
-return 0;
+return ret;
fail:
qcow2_free_snapshots(bs);
@@ -249,7 +273,7 @@ static int qcow2_open(BlockDriverState *bs, int flags)
qemu_free(s-l2_cache);
qemu_free(s-cluster_cache);
qemu_free(s-cluster_data);
-return -1;
+return ret;
}
static int qcow2_set_key(BlockDriverState *bs, const char *key)
--
1.7.3.3
[Qemu-devel] [PATCH 1/3] qemu-img.c: Re-factor img_create()
From: Jes Sorensen jes.soren...@redhat.com
This patch re-factors img_create() moving the code doing the actual
work into block.c where it can be shared with QEMU. This is needed to
be able to create images from QEMU to be used for live snapshots.
Signed-off-by: Jes Sorensen jes.soren...@redhat.com
---
block.c| 144
block.h|4 ++
qemu-img.c | 108 +
3 files changed, 150 insertions(+), 106 deletions(-)
diff --git a/block.c b/block.c
index b4aaf41..765f9f3 100644
--- a/block.c
+++ b/block.c
@@ -2758,3 +2758,147 @@ int64_t bdrv_get_dirty_count(BlockDriverState *bs)
{
return bs-dirty_count;
}
+
+int bdrv_img_create(const char *filename, const char *fmt,
+const char *base_filename, const char *base_fmt,
+char *options, uint64_t img_size, int flags)
+{
+QEMUOptionParameter *param = NULL, *create_options = NULL;
+QEMUOptionParameter *backing_fmt;
+BlockDriverState *bs = NULL;
+BlockDriver *drv, *proto_drv;
+int ret = 0;
+
+/* Find driver and parse its options */
+drv = bdrv_find_format(fmt);
+if (!drv) {
+error_report(Unknown file format '%s', fmt);
+ret = -1;
+goto out;
+}
+
+proto_drv = bdrv_find_protocol(filename);
+if (!proto_drv) {
+error_report(Unknown protocol '%s', filename);
+ret = -1;
+goto out;
+}
+
+create_options = append_option_parameters(create_options,
+ drv-create_options);
+create_options = append_option_parameters(create_options,
+ proto_drv-create_options);
+
+/* Create parameter list with default values */
+param = parse_option_parameters(, create_options, param);
+
+set_option_parameter_int(param, BLOCK_OPT_SIZE, img_size);
+
+/* Parse -o options */
+if (options) {
+param = parse_option_parameters(options, create_options, param);
+if (param == NULL) {
+error_report(Invalid options for file format '%s'., fmt);
+ret = -1;
+goto out;
+}
+}
+
+if (base_filename) {
+if (set_option_parameter(param, BLOCK_OPT_BACKING_FILE,
+ base_filename)) {
+error_report(Backing file not supported for file format '%s',
+ fmt);
+ret = -1;
+goto out;
+}
+}
+
+backing_fmt = get_option_parameter(param, BLOCK_OPT_BACKING_FMT);
+if (backing_fmt backing_fmt-value.s) {
+if (!bdrv_find_format(backing_fmt-value.s)) {
+error_report(Unknown backing file format '%s',
+ backing_fmt-value.s);
+ret = -1;
+goto out;
+}
+}
+
+if (base_fmt) {
+if (set_option_parameter(param, BLOCK_OPT_BACKING_FMT, base_fmt)) {
+error_report(Backing file format not supported for file
+ format '%s', fmt);
+ret = -1;
+goto out;
+}
+}
+
+// The size for the image must always be specified, with one exception:
+// If we are using a backing file, we can obtain the size from there
+if (get_option_parameter(param, BLOCK_OPT_SIZE)-value.n == -1) {
+QEMUOptionParameter *backing_file =
+get_option_parameter(param, BLOCK_OPT_BACKING_FILE);
+
+if (backing_file backing_file-value.s) {
+uint64_t size;
+const char *fmt = NULL;
+char buf[32];
+
+if (backing_fmt backing_fmt-value.s) {
+fmt = backing_fmt-value.s;
+}
+
+bs = bdrv_new();
+if (!bs) {
+error_report(Not enough memory to allocate BlockDriverState);
+ret = -1;
+goto out;
+}
+ret = bdrv_open(bs, backing_file-value.s, flags, drv);
+if (ret 0) {
+error_report(Could not open '%s', filename);
+ret = -1;
+goto out;
+}
+bdrv_get_geometry(bs, size);
+size *= 512;
+
+snprintf(buf, sizeof(buf), % PRId64, size);
+set_option_parameter(param, BLOCK_OPT_SIZE, buf);
+} else {
+error_report(Image creation needs a size parameter);
+ret = -1;
+goto out;
+}
+}
+
+printf(Formatting '%s', fmt=%s , filename, fmt);
+print_option_parameters(param);
+puts();
+
+ret = bdrv_create(drv, filename, param);
+free_option_parameters(create_options);
+free_option_parameters(param);
+
+if (ret 0) {
+if (ret == -ENOTSUP) {
+error_report(Formatting or formatting option not supported for
+ file format '%s', fmt);
+} else if (ret == -EFBIG
[Qemu-devel] [PATCH 3/3] Prevent creating an image with the same filename as backing file
From: Jes Sorensen jes.soren...@redhat.com
Signed-off-by: Jes Sorensen jes.soren...@redhat.com
---
block.c |7 +++
1 files changed, 7 insertions(+), 0 deletions(-)
diff --git a/block.c b/block.c
index 765f9f3..027dc6a 100644
--- a/block.c
+++ b/block.c
@@ -2769,6 +2769,13 @@ int bdrv_img_create(const char *filename, const char
*fmt,
BlockDriver *drv, *proto_drv;
int ret = 0;
+if (!strcmp(filename, base_filename)) {
+error_report(Error: Trying to create a snapshot with the same
+ filename as the backing file);
+ret = -1;
+goto out;
+}
+
/* Find driver and parse its options */
drv = bdrv_find_format(fmt);
if (!drv) {
--
1.7.3.3
[Qemu-devel] [PATCH v2 0/3] Re-factor img_create() and add live snapshots
From: Jes Sorensen jes.soren...@redhat.com Hi, This set of patches re-factors img_create() and moves the core part of it into block.c so it can be accessed from qemu as well as qemu-img. The second patch adds basic live snapshots support to the code, however only snapshots to external QCOW2 images is supported for now. QED support should be trivial once the QED patches go into upstream. The last patch fixes a small gotcha which is present in the old code as well. Try to catch cases where a user tries to create an image with itself as the backing file. QEMU does 'interesting' things when you do this. Many thanks to Kevin for his help with block layer internals! New in v2: - Fix error return value in monitor command - Clarify help message for command - Fix patch conflict against block tree. It's all Stefan's fault :) f8feb11f4d76f390dddc5cc5345abf99f7659a78 Cheers, Jes Jes Sorensen (3): qemu-img.c: Re-factor img_create() Introduce do_snapshot_blkdev() and monitor command to handle it. Prevent creating an image with the same filename as backing file block.c | 151 +++ block.h |4 ++ blockdev.c | 61 ++ blockdev.h |1 + hmp-commands.hx | 19 +++ qemu-img.c | 108 +-- 6 files changed, 238 insertions(+), 106 deletions(-) -- 1.7.3.3
[Qemu-devel] [PATCH 2/3] Introduce do_snapshot_blkdev() and monitor command to handle it.
From: Jes Sorensen jes.soren...@redhat.com
The monitor command is:
snapshot_blkdev device [snapshot-file] [format]
Default format is qcow2. For now snapshots without a snapshot-file, eg
internal snapshots, are not supported.
Signed-off-by: Jes Sorensen jes.soren...@redhat.com
---
blockdev.c | 61 +++
blockdev.h |1 +
hmp-commands.hx | 19 +
3 files changed, 81 insertions(+), 0 deletions(-)
diff --git a/blockdev.c b/blockdev.c
index 3b3b82d..9d6f72c 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -516,6 +516,67 @@ void do_commit(Monitor *mon, const QDict *qdict)
}
}
+int do_snapshot_blkdev(Monitor *mon, const QDict *qdict, QObject **ret_data)
+{
+const char *device = qdict_get_str(qdict, device);
+const char *filename = qdict_get_try_str(qdict, snapshot_file);
+const char *format = qdict_get_try_str(qdict, format);
+const char format_qcow2[] = qcow2;
+BlockDriverState *bs;
+BlockDriver *drv, *proto_drv;
+int ret = 0;
+int flags;
+
+bs = bdrv_find(device);
+if (!bs) {
+qerror_report(QERR_DEVICE_NOT_FOUND, device);
+ret = -1;
+goto out;
+}
+
+if (!format) {
+format = format_qcow2;
+}
+
+drv = bdrv_find_format(format);
+if (!drv) {
+qerror_report(QERR_INVALID_BLOCK_FORMAT, format);
+ret = -1;
+goto out;
+}
+
+proto_drv = bdrv_find_protocol(filename);
+if (!proto_drv) {
+qerror_report(QERR_INVALID_BLOCK_FORMAT, format);
+ret = -1;
+goto out;
+}
+
+ret = bdrv_img_create(filename, format, bs-filename,
+ bs-drv-format_name, NULL, -1, bs-open_flags);
+if (ret) {
+goto out;
+}
+
+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 :(
+ */
+assert(ret == 0);
+out:
+if (ret) {
+ret = -1;
+}
+
+return ret;
+}
+
static int eject_device(Monitor *mon, BlockDriverState *bs, int force)
{
if (!force) {
diff --git a/blockdev.h b/blockdev.h
index 4cb8ca9..4536b5c 100644
--- a/blockdev.h
+++ b/blockdev.h
@@ -52,5 +52,6 @@ int do_block_set_passwd(Monitor *mon, const QDict *qdict,
QObject **ret_data);
int do_change_block(Monitor *mon, const char *device,
const char *filename, const char *fmt);
int do_drive_del(Monitor *mon, const QDict *qdict, QObject **ret_data);
+int do_snapshot_blkdev(Monitor *mon, const QDict *qdict, QObject **ret_data);
#endif
diff --git a/hmp-commands.hx b/hmp-commands.hx
index 23024ba..dd3db36 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -801,6 +801,25 @@ STEXI
Set maximum tolerated downtime (in seconds) for migration.
ETEXI
+{
+.name = snapshot_blkdev,
+.args_type = device:s,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
+ new image file will become the new root image.\n\t\t\t
+ If format is specified, the snapshot file will\n\t\t\t
+ be created in that format. Otherwise the\n\t\t\t
+ snapshot will be internal! (currently unsupported),
+.mhandler.cmd_new = do_snapshot_blkdev,
+},
+
+STEXI
+...@item snapshot_blkdev
+...@findex snapshot_blkdev
+Snapshot device, using snapshot file as target if provided
+ETEXI
+
#if defined(TARGET_I386)
{
.name = drive_add,
--
1.7.3.3
[Qemu-devel] Re: [PATCH 2/3] Introduce do_snapshot_blkdev() and monitor command to handle it.
On 12/16/10 12:45, Kevin Wolf wrote:
Am 16.12.2010 12:04, schrieb jes.soren...@redhat.com:
From: Jes Sorensen jes.soren...@redhat.com
The monitor command is:
snapshot_blkdev device [snapshot-file] [format]
Default format is qcow2. For now snapshots without a snapshot-file, eg
internal snapshots, are not supported.
Signed-off-by: Jes Sorensen jes.soren...@redhat.com
---
blockdev.c | 61
+++
blockdev.h |1 +
hmp-commands.hx | 19 +
3 files changed, 81 insertions(+), 0 deletions(-)
diff --git a/blockdev.c b/blockdev.c
index 3b3b82d..9d6f72c 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -516,6 +516,67 @@ void do_commit(Monitor *mon, const QDict *qdict)
}
}
+int do_snapshot_blkdev(Monitor *mon, const QDict *qdict, QObject **ret_data)
+{
+const char *device = qdict_get_str(qdict, device);
+const char *filename = qdict_get_try_str(qdict, snapshot_file);
+const char *format = qdict_get_try_str(qdict, format);
+const char format_qcow2[] = qcow2;
+BlockDriverState *bs;
+BlockDriver *drv, *proto_drv;
+int ret = 0;
+int flags;
+
+bs = bdrv_find(device);
+if (!bs) {
+qerror_report(QERR_DEVICE_NOT_FOUND, device);
+ret = -1;
+goto out;
+}
+
+if (!format) {
+format = format_qcow2;
Why introducing format_qcow2 instead of directly using the string
literal here?
It should generate the same code - I kinda liked my style better, but
I'll change it.
+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 :(
+ */
+assert(ret == 0);
bdrv_commit handles this case by setting bs-drv = NULL. After this the
device will fail all requests with -ENOMEDIUM, but at least you don't
lose your VM immediately.
Well if we hit this situation something catastrophic happened. I don't
see how we can continue beyond this point really. The old image was
dropped in the swap and the new one is not accessible ... we're dead :(
Cheers,
Jes
[Qemu-devel] Re: [PATCH 1/3] qemu-img.c: Re-factor img_create()
On 12/16/10 12:35, Kevin Wolf wrote:
Am 16.12.2010 12:04, schrieb jes.soren...@redhat.com:
+
+backing_fmt = get_option_parameter(param, BLOCK_OPT_BACKING_FMT);
+if (backing_fmt backing_fmt-value.s) {
+if (!bdrv_find_format(backing_fmt-value.s)) {
+error_report(Unknown backing file format '%s',
+ backing_fmt-value.s);
+ret = -1;
+goto out;
+}
+}
+
+if (base_fmt) {
+if (set_option_parameter(param, BLOCK_OPT_BACKING_FMT, base_fmt)) {
+error_report(Backing file format not supported for file
+ format '%s', fmt);
+ret = -1;
+goto out;
+}
+}
The order is wrong here. If you use -F, the backing format won't be checked.
Urgh yes, my bad! I had it the other way, but decided to change it -
very silly.
+
+// The size for the image must always be specified, with one exception:
+// If we are using a backing file, we can obtain the size from there
+if (get_option_parameter(param, BLOCK_OPT_SIZE)-value.n == -1) {
+QEMUOptionParameter *backing_file =
+get_option_parameter(param, BLOCK_OPT_BACKING_FILE);
+
+if (backing_file backing_file-value.s) {
+uint64_t size;
+const char *fmt = NULL;
+char buf[32];
+
+if (backing_fmt backing_fmt-value.s) {
+fmt = backing_fmt-value.s;
+}
+
+bs = bdrv_new();
+if (!bs) {
+error_report(Not enough memory to allocate
BlockDriverState);
+ret = -1;
+goto out;
+}
bdrv_new never returns NULL (it's an indirect qemu_malloc call).
I see - this was copied wholesale from qemu-img.c but I'll just remove
the error check.
+ret = bdrv_open(bs, backing_file-value.s, flags, drv);
+if (ret 0) {
+error_report(Could not open '%s', filename);
+ret = -1;
+goto out;
+}
+bdrv_get_geometry(bs, size);
+size *= 512;
+
+snprintf(buf, sizeof(buf), % PRId64, size);
+set_option_parameter(param, BLOCK_OPT_SIZE, buf);
+} else {
+error_report(Image creation needs a size parameter);
+ret = -1;
+goto out;
+}
+}
+
+printf(Formatting '%s', fmt=%s , filename, fmt);
+print_option_parameters(param);
+puts();
+
+ret = bdrv_create(drv, filename, param);
+free_option_parameters(create_options);
+free_option_parameters(param);
These need to be after out: to avoid leaking in error cases.
You're basically reverting a87a6721d with this.
Whoops - another one of those conflicting ones. It's all Stefan's fault :)
+
+if (ret 0) {
+if (ret == -ENOTSUP) {
+error_report(Formatting or formatting option not supported for
+ file format '%s', fmt);
+} else if (ret == -EFBIG) {
+error_report(The image size is too large for file format '%s',
+ fmt);
+} else {
+error_report(%s: error while creating %s: %s, filename, fmt,
+ strerror(-ret));
+}
+}
+
+out:
+if (bs) {
+bdrv_delete(bs);
+}
+if (ret) {
+return 1;
+}
Maybe we should better use the usual 0/-errno style. In qemu-img it was
the exit code of the program, but now it's a block layer function.
I like errno, I made it behave like this for consistency with the rest
of QEMU. In most places we don't seem to like anything but -1/1 for
error values.
Let me know what you prefer, or alternatively we can change it in a
follow-on patch?
Cheers,
Jes
[Qemu-devel] [PATCH 2/4] Introduce do_snapshot_blkdev() and monitor command to handle it.
From: Jes Sorensen jes.soren...@redhat.com
The monitor command is:
snapshot_blkdev device [snapshot-file] [format]
Default format is qcow2. For now snapshots without a snapshot-file, eg
internal snapshots, are not supported.
Signed-off-by: Jes Sorensen jes.soren...@redhat.com
---
blockdev.c | 62 +++
blockdev.h |1 +
hmp-commands.hx | 19
3 files changed, 82 insertions(+), 0 deletions(-)
diff --git a/blockdev.c b/blockdev.c
index 3b3b82d..d7add36 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -516,6 +516,68 @@ void do_commit(Monitor *mon, const QDict *qdict)
}
}
+int do_snapshot_blkdev(Monitor *mon, const QDict *qdict, QObject **ret_data)
+{
+const char *device = qdict_get_str(qdict, device);
+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;
+int ret = 0;
+int flags;
+
+bs = bdrv_find(device);
+if (!bs) {
+qerror_report(QERR_DEVICE_NOT_FOUND, device);
+ret = -1;
+goto out;
+}
+
+if (!format) {
+format = qcow2;
+}
+
+drv = bdrv_find_format(format);
+if (!drv) {
+qerror_report(QERR_INVALID_BLOCK_FORMAT, format);
+ret = -1;
+goto out;
+}
+
+proto_drv = bdrv_find_protocol(filename);
+if (!proto_drv) {
+qerror_report(QERR_INVALID_BLOCK_FORMAT, format);
+ret = -1;
+goto out;
+}
+
+ret = bdrv_img_create(filename, format, bs-filename,
+ bs-drv-format_name, NULL, -1, bs-open_flags);
+if (ret) {
+goto out;
+}
+
+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 (ret != 0) {
+abort();
+}
+out:
+if (ret) {
+ret = -1;
+}
+
+return ret;
+}
+
static int eject_device(Monitor *mon, BlockDriverState *bs, int force)
{
if (!force) {
diff --git a/blockdev.h b/blockdev.h
index 4cb8ca9..4536b5c 100644
--- a/blockdev.h
+++ b/blockdev.h
@@ -52,5 +52,6 @@ int do_block_set_passwd(Monitor *mon, const QDict *qdict,
QObject **ret_data);
int do_change_block(Monitor *mon, const char *device,
const char *filename, const char *fmt);
int do_drive_del(Monitor *mon, const QDict *qdict, QObject **ret_data);
+int do_snapshot_blkdev(Monitor *mon, const QDict *qdict, QObject **ret_data);
#endif
diff --git a/hmp-commands.hx b/hmp-commands.hx
index 23024ba..dd3db36 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -801,6 +801,25 @@ STEXI
Set maximum tolerated downtime (in seconds) for migration.
ETEXI
+{
+.name = snapshot_blkdev,
+.args_type = device:s,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
+ new image file will become the new root image.\n\t\t\t
+ If format is specified, the snapshot file will\n\t\t\t
+ be created in that format. Otherwise the\n\t\t\t
+ snapshot will be internal! (currently unsupported),
+.mhandler.cmd_new = do_snapshot_blkdev,
+},
+
+STEXI
+...@item snapshot_blkdev
+...@findex snapshot_blkdev
+Snapshot device, using snapshot file as target if provided
+ETEXI
+
#if defined(TARGET_I386)
{
.name = drive_add,
--
1.7.3.3
[Qemu-devel] [PATCH 1/4] qemu-img.c: Re-factor img_create()
From: Jes Sorensen jes.soren...@redhat.com
This patch re-factors img_create() moving the code doing the actual
work into block.c where it can be shared with QEMU. This is needed to
be able to create images from QEMU to be used for live snapshots.
Signed-off-by: Jes Sorensen jes.soren...@redhat.com
---
block.c| 141
block.h|4 ++
qemu-img.c | 108 +-
3 files changed, 147 insertions(+), 106 deletions(-)
diff --git a/block.c b/block.c
index b4aaf41..a48b30c 100644
--- a/block.c
+++ b/block.c
@@ -2758,3 +2758,144 @@ int64_t bdrv_get_dirty_count(BlockDriverState *bs)
{
return bs-dirty_count;
}
+
+int bdrv_img_create(const char *filename, const char *fmt,
+const char *base_filename, const char *base_fmt,
+char *options, uint64_t img_size, int flags)
+{
+QEMUOptionParameter *param = NULL, *create_options = NULL;
+QEMUOptionParameter *backing_fmt;
+BlockDriverState *bs = NULL;
+BlockDriver *drv, *proto_drv;
+int ret = 0;
+
+/* Find driver and parse its options */
+drv = bdrv_find_format(fmt);
+if (!drv) {
+error_report(Unknown file format '%s', fmt);
+ret = -1;
+goto out;
+}
+
+proto_drv = bdrv_find_protocol(filename);
+if (!proto_drv) {
+error_report(Unknown protocol '%s', filename);
+ret = -1;
+goto out;
+}
+
+create_options = append_option_parameters(create_options,
+ drv-create_options);
+create_options = append_option_parameters(create_options,
+ proto_drv-create_options);
+
+/* Create parameter list with default values */
+param = parse_option_parameters(, create_options, param);
+
+set_option_parameter_int(param, BLOCK_OPT_SIZE, img_size);
+
+/* Parse -o options */
+if (options) {
+param = parse_option_parameters(options, create_options, param);
+if (param == NULL) {
+error_report(Invalid options for file format '%s'., fmt);
+ret = -1;
+goto out;
+}
+}
+
+if (base_filename) {
+if (set_option_parameter(param, BLOCK_OPT_BACKING_FILE,
+ base_filename)) {
+error_report(Backing file not supported for file format '%s',
+ fmt);
+ret = -1;
+goto out;
+}
+}
+
+if (base_fmt) {
+if (set_option_parameter(param, BLOCK_OPT_BACKING_FMT, base_fmt)) {
+error_report(Backing file format not supported for file
+ format '%s', fmt);
+ret = -1;
+goto out;
+}
+}
+
+backing_fmt = get_option_parameter(param, BLOCK_OPT_BACKING_FMT);
+if (backing_fmt backing_fmt-value.s) {
+if (!bdrv_find_format(backing_fmt-value.s)) {
+error_report(Unknown backing file format '%s',
+ backing_fmt-value.s);
+ret = -1;
+goto out;
+}
+}
+
+// The size for the image must always be specified, with one exception:
+// If we are using a backing file, we can obtain the size from there
+if (get_option_parameter(param, BLOCK_OPT_SIZE)-value.n == -1) {
+QEMUOptionParameter *backing_file =
+get_option_parameter(param, BLOCK_OPT_BACKING_FILE);
+
+if (backing_file backing_file-value.s) {
+uint64_t size;
+const char *fmt = NULL;
+char buf[32];
+
+if (backing_fmt backing_fmt-value.s) {
+fmt = backing_fmt-value.s;
+}
+
+bs = bdrv_new();
+
+ret = bdrv_open(bs, backing_file-value.s, flags, drv);
+if (ret 0) {
+error_report(Could not open '%s', filename);
+ret = -1;
+goto out;
+}
+bdrv_get_geometry(bs, size);
+size *= 512;
+
+snprintf(buf, sizeof(buf), % PRId64, size);
+set_option_parameter(param, BLOCK_OPT_SIZE, buf);
+} else {
+error_report(Image creation needs a size parameter);
+ret = -1;
+goto out;
+}
+}
+
+printf(Formatting '%s', fmt=%s , filename, fmt);
+print_option_parameters(param);
+puts();
+
+ret = bdrv_create(drv, filename, param);
+
+if (ret 0) {
+if (ret == -ENOTSUP) {
+error_report(Formatting or formatting option not supported for
+ file format '%s', fmt);
+} else if (ret == -EFBIG) {
+error_report(The image size is too large for file format '%s',
+ fmt);
+} else {
+error_report(%s: error while creating %s: %s, filename, fmt,
+ strerror(-ret
[Qemu-devel] [PATCH v3 0/4] Re-factor img_create() and add live snapshots
From: Jes Sorensen jes.soren...@redhat.com Hi, This set of patches re-factors img_create() and moves the core part of it into block.c so it can be accessed from qemu as well as qemu-img. The second patch adds basic live snapshots support to the code, however only snapshots to external QCOW2 images is supported for now. QED support should be trivial once the QED patches go into upstream. The last patch fixes a small gotcha which is present in the old code as well. Try to catch cases where a user tries to create an image with itself as the backing file. QEMU does 'interesting' things when you do this. Many thanks to Kevin for his help with block layer internals! New in v2: - Fix error return value in monitor command - Clarify help message for command - Fix patch conflict against block tree. It's all Stefan's fault :) f8feb11f4d76f390dddc5cc5345abf99f7659a78 New in v3: - Address issues pointed out by Stefan and Kevin - Additional patch to return proper -errno error values on error in bdrv_img_create() as suggested by Kevin. Jes Sorensen (4): qemu-img.c: Re-factor img_create() Introduce do_snapshot_blkdev() and monitor command to handle it. Prevent creating an image with the same filename as backing file bdrv_img_create() use proper errno return values block.c | 145 +++ block.h |4 ++ blockdev.c | 62 +++ blockdev.h |1 + hmp-commands.hx | 19 +++ qemu-img.c | 108 + 6 files changed, 233 insertions(+), 106 deletions(-) -- 1.7.3.3
[Qemu-devel] [PATCH 4/4] bdrv_img_create() use proper errno return values
From: Jes Sorensen jes.soren...@redhat.com
Kevin suggested to have bdrv_img_create() return proper -errno values
on error.
Signed-off-by: Jes Sorensen jes.soren...@redhat.com
---
block.c | 23 ++-
1 files changed, 10 insertions(+), 13 deletions(-)
diff --git a/block.c b/block.c
index 0c14eee..fe07d0b 100644
--- a/block.c
+++ b/block.c
@@ -2773,14 +2773,14 @@ int bdrv_img_create(const char *filename, const char
*fmt,
drv = bdrv_find_format(fmt);
if (!drv) {
error_report(Unknown file format '%s', fmt);
-ret = -1;
+ret = -EINVAL;
goto out;
}
proto_drv = bdrv_find_protocol(filename);
if (!proto_drv) {
error_report(Unknown protocol '%s', filename);
-ret = -1;
+ret = -EINVAL;
goto out;
}
@@ -2799,7 +2799,7 @@ int bdrv_img_create(const char *filename, const char *fmt,
param = parse_option_parameters(options, create_options, param);
if (param == NULL) {
error_report(Invalid options for file format '%s'., fmt);
-ret = -1;
+ret = -EINVAL;
goto out;
}
}
@@ -2809,7 +2809,7 @@ int bdrv_img_create(const char *filename, const char *fmt,
base_filename)) {
error_report(Backing file not supported for file format '%s',
fmt);
-ret = -1;
+ret = -EINVAL;
goto out;
}
}
@@ -2818,7 +2818,7 @@ int bdrv_img_create(const char *filename, const char *fmt,
if (set_option_parameter(param, BLOCK_OPT_BACKING_FMT, base_fmt)) {
error_report(Backing file format not supported for file
format '%s', fmt);
-ret = -1;
+ret = -EINVAL;
goto out;
}
}
@@ -2828,7 +2828,7 @@ int bdrv_img_create(const char *filename, const char *fmt,
if (!strcmp(filename, backing_file-value.s)) {
error_report(Error: Trying to create an image with the
same filename as the backing file);
-ret = -1;
+ret = -EINVAL;
goto out;
}
}
@@ -2838,7 +2838,7 @@ int bdrv_img_create(const char *filename, const char *fmt,
if (!bdrv_find_format(backing_fmt-value.s)) {
error_report(Unknown backing file format '%s',
backing_fmt-value.s);
-ret = -1;
+ret = -EINVAL;
goto out;
}
}
@@ -2860,7 +2860,6 @@ int bdrv_img_create(const char *filename, const char *fmt,
ret = bdrv_open(bs, backing_file-value.s, flags, drv);
if (ret 0) {
error_report(Could not open '%s', filename);
-ret = -1;
goto out;
}
bdrv_get_geometry(bs, size);
@@ -2870,7 +2869,7 @@ int bdrv_img_create(const char *filename, const char *fmt,
set_option_parameter(param, BLOCK_OPT_SIZE, buf);
} else {
error_report(Image creation needs a size parameter);
-ret = -1;
+ret = -EINVAL;
goto out;
}
}
@@ -2901,8 +2900,6 @@ out:
if (bs) {
bdrv_delete(bs);
}
-if (ret) {
-return 1;
-}
-return 0;
+
+return ret;
}
--
1.7.3.3
[Qemu-devel] [PATCH 3/4] Prevent creating an image with the same filename as backing file
From: Jes Sorensen jes.soren...@redhat.com
Signed-off-by: Jes Sorensen jes.soren...@redhat.com
---
block.c | 15 +++
1 files changed, 11 insertions(+), 4 deletions(-)
diff --git a/block.c b/block.c
index a48b30c..0c14eee 100644
--- a/block.c
+++ b/block.c
@@ -2764,7 +2764,7 @@ int bdrv_img_create(const char *filename, const char *fmt,
char *options, uint64_t img_size, int flags)
{
QEMUOptionParameter *param = NULL, *create_options = NULL;
-QEMUOptionParameter *backing_fmt;
+QEMUOptionParameter *backing_fmt, *backing_file;
BlockDriverState *bs = NULL;
BlockDriver *drv, *proto_drv;
int ret = 0;
@@ -2823,6 +2823,16 @@ int bdrv_img_create(const char *filename, const char
*fmt,
}
}
+backing_file = get_option_parameter(param, BLOCK_OPT_BACKING_FILE);
+if (backing_file backing_file-value.s) {
+if (!strcmp(filename, backing_file-value.s)) {
+error_report(Error: Trying to create an image with the
+ same filename as the backing file);
+ret = -1;
+goto out;
+}
+}
+
backing_fmt = get_option_parameter(param, BLOCK_OPT_BACKING_FMT);
if (backing_fmt backing_fmt-value.s) {
if (!bdrv_find_format(backing_fmt-value.s)) {
@@ -2836,9 +2846,6 @@ int bdrv_img_create(const char *filename, const char *fmt,
// The size for the image must always be specified, with one exception:
// If we are using a backing file, we can obtain the size from there
if (get_option_parameter(param, BLOCK_OPT_SIZE)-value.n == -1) {
-QEMUOptionParameter *backing_file =
-get_option_parameter(param, BLOCK_OPT_BACKING_FILE);
-
if (backing_file backing_file-value.s) {
uint64_t size;
const char *fmt = NULL;
--
1.7.3.3
[Qemu-devel] [PATCH] qemu.img.c: Use error_report() instead of own error() implementation
From: Jes Sorensen jes.soren...@redhat.com
Signed-off-by: Jes Sorensen jes.soren...@redhat.com
---
qemu-img.c | 128 +--
1 files changed, 63 insertions(+), 65 deletions(-)
diff --git a/qemu-img.c b/qemu-img.c
index 603bdb3..d3921b6 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -26,6 +26,7 @@
#include osdep.h
#include sysemu.h
#include block_int.h
+#include qerror.h
#include stdio.h
#ifdef _WIN32
@@ -40,16 +41,6 @@ typedef struct img_cmd_t {
/* Default to cache=writeback as data integrity is not important for qemu-tcg.
*/
#define BDRV_O_FLAGS BDRV_O_CACHE_WB
-static void GCC_FMT_ATTR(1, 2) error(const char *fmt, ...)
-{
-va_list ap;
-va_start(ap, fmt);
-fprintf(stderr, qemu-img: );
-vfprintf(stderr, fmt, ap);
-fprintf(stderr, \n);
-va_end(ap);
-}
-
static void format_print(void *opaque, const char *name)
{
printf( %s, name);
@@ -196,13 +187,13 @@ static int print_block_option_help(const char *filename,
const char *fmt)
/* Find driver and parse its options */
drv = bdrv_find_format(fmt);
if (!drv) {
-error(Unknown file format '%s', fmt);
+error_report(Unknown file format '%s', fmt);
return 1;
}
proto_drv = bdrv_find_protocol(filename);
if (!proto_drv) {
-error(Unknown protocol '%s', filename);
+error_report(Unknown protocol '%s', filename);
return 1;
}
@@ -225,30 +216,30 @@ static BlockDriverState *bdrv_new_open(const char
*filename,
bs = bdrv_new();
if (!bs) {
-error(Not enough memory);
+error_report(Not enough memory);
goto fail;
}
if (fmt) {
drv = bdrv_find_format(fmt);
if (!drv) {
-error(Unknown file format '%s', fmt);
+error_report(Unknown file format '%s', fmt);
goto fail;
}
} else {
drv = NULL;
}
if (bdrv_open(bs, filename, flags, drv) 0) {
-error(Could not open '%s', filename);
+error_report(Could not open '%s', filename);
goto fail;
}
if (bdrv_is_encrypted(bs)) {
printf(Disk image '%s' is encrypted.\n, filename);
if (read_password(password, sizeof(password)) 0) {
-error(No password given);
+error_report(No password given);
goto fail;
}
if (bdrv_set_key(bs, password) 0) {
-error(invalid password);
+error_report(invalid password);
goto fail;
}
}
@@ -266,13 +257,15 @@ static int add_old_style_options(const char *fmt,
QEMUOptionParameter *list,
{
if (base_filename) {
if (set_option_parameter(list, BLOCK_OPT_BACKING_FILE, base_filename))
{
-error(Backing file not supported for file format '%s', fmt);
+error_report(Backing file not supported for file format '%s',
+ fmt);
return -1;
}
}
if (base_fmt) {
if (set_option_parameter(list, BLOCK_OPT_BACKING_FMT, base_fmt)) {
-error(Backing file format not supported for file format '%s',
fmt);
+error_report(Backing file format not supported for file
+ format '%s', fmt);
return -1;
}
}
@@ -309,11 +302,11 @@ static int img_create(int argc, char **argv)
fmt = optarg;
break;
case 'e':
-error(qemu-img: option -e is deprecated, please use \'-o
+error_report(qemu-img: option -e is deprecated, please use \'-o
encryption\' instead!);
return 1;
case '6':
-error(qemu-img: option -6 is deprecated, please use \'-o
+error_report(qemu-img: option -6 is deprecated, please use \'-o
compat6\' instead!);
return 1;
case 'o':
@@ -333,9 +326,9 @@ static int img_create(int argc, char **argv)
ssize_t sval;
sval = strtosz_suffix(argv[optind++], NULL, STRTOSZ_DEFSUFFIX_B);
if (sval 0) {
-error(Invalid image size specified! You may use k, M, G or
+error_report(Invalid image size specified! You may use k, M, G or
T suffixes for );
-error(kilobytes, megabytes, gigabytes and terabytes.);
+error_report(kilobytes, megabytes, gigabytes and terabytes.);
ret = -1;
goto out;
}
@@ -399,7 +392,7 @@ static int img_check(int argc, char **argv)
ret = bdrv_check(bs, result);
if (ret == -ENOTSUP) {
-error(This image format does not support checks);
+error_report(This image format does not support checks);
bdrv_delete(bs);
return 1;
}
@@ -481,16 +474,16 @@ static int img_commit(int argc, char **argv)
printf(Image committed.\n);
break;
case -ENOENT:
-error(No disk
[Qemu-devel] Re: [PATCH] qemu-img: Call error_set_progname
On 12/16/10 15:13, Kevin Wolf wrote: Call error_set_progname during the qemu-img initialization, so that error messages printed with error_report() use the right prefix. Signed-off-by: Kevin Wolf kw...@redhat.com Acked-by: Jes Sorensen jes.soren...@redhat.com --- qemu-img.c |3 +++ 1 files changed, 3 insertions(+), 0 deletions(-) diff --git a/qemu-img.c b/qemu-img.c index 603bdb3..0ff179f 100644 --- a/qemu-img.c +++ b/qemu-img.c @@ -23,6 +23,7 @@ */ #include qemu-common.h #include qemu-option.h +#include qemu-error.h #include osdep.h #include sysemu.h #include block_int.h @@ -1508,6 +1509,8 @@ int main(int argc, char **argv) const img_cmd_t *cmd; const char *cmdname; +error_set_progname(argv[0]); + bdrv_init(); if (argc 2) help();
[Qemu-devel] [PATCH 2/2] Add proper -errno error return values to qcow2_open()
From: Jes Sorensen jes.soren...@redhat.com
In addition this adds missing braces to the function to be consistent
with the coding style.
Signed-off-by: Jes Sorensen jes.soren...@redhat.com
---
block/qcow2.c | 61
1 files changed, 43 insertions(+), 18 deletions(-)
diff --git a/block/qcow2.c b/block/qcow2.c
index d7fd167..b4a9e5e 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -140,12 +140,14 @@ static int qcow2_read_extensions(BlockDriverState *bs,
uint64_t start_offset,
static int qcow2_open(BlockDriverState *bs, int flags)
{
BDRVQcowState *s = bs-opaque;
-int len, i;
+int len, i, ret = 0;
QCowHeader header;
uint64_t ext_end;
-if (bdrv_pread(bs-file, 0, header, sizeof(header)) != sizeof(header))
+if (bdrv_pread(bs-file, 0, header, sizeof(header)) != sizeof(header)) {
+ret = -EIO;
goto fail;
+}
be32_to_cpus(header.magic);
be32_to_cpus(header.version);
be64_to_cpus(header.backing_file_offset);
@@ -160,16 +162,23 @@ static int qcow2_open(BlockDriverState *bs, int flags)
be64_to_cpus(header.snapshots_offset);
be32_to_cpus(header.nb_snapshots);
-if (header.magic != QCOW_MAGIC || header.version != QCOW_VERSION)
+if (header.magic != QCOW_MAGIC || header.version != QCOW_VERSION) {
+ret = -EINVAL;
goto fail;
+}
if (header.cluster_bits MIN_CLUSTER_BITS ||
-header.cluster_bits MAX_CLUSTER_BITS)
+header.cluster_bits MAX_CLUSTER_BITS) {
+ret = -EINVAL;
goto fail;
-if (header.crypt_method QCOW_CRYPT_AES)
+}
+if (header.crypt_method QCOW_CRYPT_AES) {
+ret = -EINVAL;
goto fail;
+}
s-crypt_method_header = header.crypt_method;
-if (s-crypt_method_header)
+if (s-crypt_method_header) {
bs-encrypted = 1;
+}
s-cluster_bits = header.cluster_bits;
s-cluster_size = 1 s-cluster_bits;
s-cluster_sectors = 1 (s-cluster_bits - 9);
@@ -191,15 +200,20 @@ static int qcow2_open(BlockDriverState *bs, int flags)
s-l1_vm_state_index = size_to_l1(s, header.size);
/* the L1 table must contain at least enough entries to put
header.size bytes */
-if (s-l1_size s-l1_vm_state_index)
+if (s-l1_size s-l1_vm_state_index) {
+ret = -EINVAL;
goto fail;
+}
s-l1_table_offset = header.l1_table_offset;
if (s-l1_size 0) {
s-l1_table = qemu_mallocz(
align_offset(s-l1_size * sizeof(uint64_t), 512));
-if (bdrv_pread(bs-file, s-l1_table_offset, s-l1_table, s-l1_size *
sizeof(uint64_t)) !=
-s-l1_size * sizeof(uint64_t))
+if (bdrv_pread(bs-file, s-l1_table_offset, s-l1_table,
+ s-l1_size * sizeof(uint64_t)) !=
+s-l1_size * sizeof(uint64_t)) {
+ret = -EIO;
goto fail;
+}
for(i = 0;i s-l1_size; i++) {
be64_to_cpus(s-l1_table[i]);
}
@@ -212,35 +226,46 @@ static int qcow2_open(BlockDriverState *bs, int flags)
+ 512);
s-cluster_cache_offset = -1;
-if (qcow2_refcount_init(bs) 0)
+ret = qcow2_refcount_init(bs);
+if (ret != 0) {
goto fail;
+}
QLIST_INIT(s-cluster_allocs);
/* read qcow2 extensions */
-if (header.backing_file_offset)
+if (header.backing_file_offset) {
ext_end = header.backing_file_offset;
-else
+} else {
ext_end = s-cluster_size;
-if (qcow2_read_extensions(bs, sizeof(header), ext_end))
+}
+if (qcow2_read_extensions(bs, sizeof(header), ext_end)) {
+ret = -EINVAL;
goto fail;
+}
/* read the backing file name */
if (header.backing_file_offset != 0) {
len = header.backing_file_size;
-if (len 1023)
+if (len 1023) {
len = 1023;
-if (bdrv_pread(bs-file, header.backing_file_offset, bs-backing_file,
len) != len)
+}
+if (bdrv_pread(bs-file, header.backing_file_offset,
+ bs-backing_file, len) != len) {
+ret = -EIO;
goto fail;
+}
bs-backing_file[len] = '\0';
}
-if (qcow2_read_snapshots(bs) 0)
+if (qcow2_read_snapshots(bs) 0) {
+ret = -EINVAL;
goto fail;
+}
#ifdef DEBUG_ALLOC
qcow2_check_refcounts(bs);
#endif
-return 0;
+return ret;
fail:
qcow2_free_snapshots(bs);
@@ -249,7 +274,7 @@ static int qcow2_open(BlockDriverState *bs, int flags)
qemu_free(s-l2_cache);
qemu_free(s-cluster_cache);
qemu_free(s-cluster_data);
-return -1;
+return ret;
}
static int qcow2_set_key(BlockDriverState *bs, const char *key)
--
1.7.3.3
[Qemu-devel] [PATCH 0/2] qcow2 cleanups
From: Jes Sorensen jes.soren...@redhat.com Hi, These two patches tries to clean up the qcow2 code a little. First makes the function names consistent, ie. we shouldn't have qcow_ functions in the qcow2 code. Second tries to add proper errno return values to qcow2_open(). Jes Sorensen (2): block/qcow2.c: rename qcow_ functions to qcow2_ Add proper -errno error return values to qcow2_open() block/qcow2-cluster.c |6 +- block/qcow2-snapshot.c |6 +- block/qcow2.c | 269 +++- 3 files changed, 158 insertions(+), 123 deletions(-) -- 1.7.3.3
[Qemu-devel] [PATCH 1/2] block/qcow2.c: rename qcow_ functions to qcow2_
From: Jes Sorensen jes.soren...@redhat.com
It doesn't really make sense for functions in qcow2.c to be named
qcow_ so convert the names to match correctly.
Signed-off-by: Jes Sorensen jes.soren...@redhat.com
---
block/qcow2-cluster.c |6 +-
block/qcow2-snapshot.c |6 +-
block/qcow2.c | 210 +---
3 files changed, 116 insertions(+), 106 deletions(-)
diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index b040208..6928c63 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -352,8 +352,8 @@ void qcow2_encrypt_sectors(BDRVQcowState *s, int64_t
sector_num,
}
-static int qcow_read(BlockDriverState *bs, int64_t sector_num,
- uint8_t *buf, int nb_sectors)
+static int qcow2_read(BlockDriverState *bs, int64_t sector_num,
+ uint8_t *buf, int nb_sectors)
{
BDRVQcowState *s = bs-opaque;
int ret, index_in_cluster, n, n1;
@@ -419,7 +419,7 @@ static int copy_sectors(BlockDriverState *bs, uint64_t
start_sect,
if (n = 0)
return 0;
BLKDBG_EVENT(bs-file, BLKDBG_COW_READ);
-ret = qcow_read(bs, start_sect + n_start, s-cluster_data, n);
+ret = qcow2_read(bs, start_sect + n_start, s-cluster_data, n);
if (ret 0)
return ret;
if (s-crypt_method) {
diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
index aacf357..74823a5 100644
--- a/block/qcow2-snapshot.c
+++ b/block/qcow2-snapshot.c
@@ -116,7 +116,7 @@ int qcow2_read_snapshots(BlockDriverState *bs)
}
/* add at the end of the file a new list of snapshots */
-static int qcow_write_snapshots(BlockDriverState *bs)
+static int qcow2_write_snapshots(BlockDriverState *bs)
{
BDRVQcowState *s = bs-opaque;
QCowSnapshot *sn;
@@ -300,7 +300,7 @@ int qcow2_snapshot_create(BlockDriverState *bs,
QEMUSnapshotInfo *sn_info)
s-snapshots = snapshots1;
s-snapshots[s-nb_snapshots++] = *sn;
-if (qcow_write_snapshots(bs) 0)
+if (qcow2_write_snapshots(bs) 0)
goto fail;
#ifdef DEBUG_ALLOC
qcow2_check_refcounts(bs);
@@ -378,7 +378,7 @@ int qcow2_snapshot_delete(BlockDriverState *bs, const char
*snapshot_id)
qemu_free(sn-name);
memmove(sn, sn + 1, (s-nb_snapshots - snapshot_index - 1) * sizeof(*sn));
s-nb_snapshots--;
-ret = qcow_write_snapshots(bs);
+ret = qcow2_write_snapshots(bs);
if (ret 0) {
/* XXX: restore snapshot if error ? */
return ret;
diff --git a/block/qcow2.c b/block/qcow2.c
index 537c479..d7fd167 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -50,10 +50,10 @@ typedef struct {
uint32_t magic;
uint32_t len;
} QCowExtension;
-#define QCOW_EXT_MAGIC_END 0
-#define QCOW_EXT_MAGIC_BACKING_FORMAT 0xE2792ACA
+#define QCOW2_EXT_MAGIC_END 0
+#define QCOW2_EXT_MAGIC_BACKING_FORMAT 0xE2792ACA
-static int qcow_probe(const uint8_t *buf, int buf_size, const char *filename)
+static int qcow2_probe(const uint8_t *buf, int buf_size, const char *filename)
{
const QCowHeader *cow_header = (const void *)buf;
@@ -73,14 +73,14 @@ static int qcow_probe(const uint8_t *buf, int buf_size,
const char *filename)
* unknown magic is skipped (future extension this version knows nothing about)
* return 0 upon success, non-0 otherwise
*/
-static int qcow_read_extensions(BlockDriverState *bs, uint64_t start_offset,
-uint64_t end_offset)
+static int qcow2_read_extensions(BlockDriverState *bs, uint64_t start_offset,
+ uint64_t end_offset)
{
QCowExtension ext;
uint64_t offset;
#ifdef DEBUG_EXT
-printf(qcow_read_extensions: start=%ld end=%ld\n, start_offset,
end_offset);
+printf(qcow2_read_extensions: start=%ld end=%ld\n, start_offset,
end_offset);
#endif
offset = start_offset;
while (offset end_offset) {
@@ -88,13 +88,13 @@ static int qcow_read_extensions(BlockDriverState *bs,
uint64_t start_offset,
#ifdef DEBUG_EXT
/* Sanity check */
if (offset s-cluster_size)
-printf(qcow_handle_extension: suspicious offset %lu\n, offset);
+printf(qcow_read_extension: suspicious offset %lu\n, offset);
printf(attemting to read extended header in offset %lu\n, offset);
#endif
if (bdrv_pread(bs-file, offset, ext, sizeof(ext)) != sizeof(ext)) {
-fprintf(stderr, qcow_handle_extension: ERROR:
+fprintf(stderr, qcow_read_extension: ERROR:
pread fail from offset % PRIu64 \n,
offset);
return 1;
@@ -106,10 +106,10 @@ static int qcow_read_extensions(BlockDriverState *bs,
uint64_t start_offset,
printf(ext.magic = 0x%x\n, ext.magic);
#endif
switch (ext.magic) {
-case QCOW_EXT_MAGIC_END:
+case QCOW2_EXT_MAGIC_END:
return 0;
-case QCOW_EXT_MAGIC_BACKING_FORMAT:
+case QCOW2_EXT_MAGIC_BACKING_FORMAT
Re: [Qemu-devel] [PATCH v2 1/1] qemu-img.c: Clean up handling of image size in img_create()
On 12/15/10 17:47, Markus Armbruster wrote: jes.soren...@redhat.com writes: From: Jes Sorensen jes.soren...@redhat.com This cleans up the handling of image size in img_create() by parsing the value early, and then only setting it once if a value has been added as the last argument to the command line. Signed-off-by: Jes Sorensen jes.soren...@redhat.com --- qemu-img.c | 14 -- 1 files changed, 8 insertions(+), 6 deletions(-) Patch conflicts with commit c2abccec. ARGH :( This switches parsing of the size argument from parse_option_size() (via set_option_parameter()) to strtosz(). I'm fine with that, but: * Before: $ qemu-img create xxx xxx Parameter 'size' expects a size You may use k, M, G or T suffixes for kilobytes, megabytes, gigabytes and terabytes. qemu-img: Image creation needs a size parameter * After: $ qemu-img create xxx xxx qemu-img: Image creation needs a size parameter Intentional? This was addressed in the later revision when I introduced strtosz_suffix() Cheers, Jes
[Qemu-devel] Re: [PATCH 2/3] Introduce do_snapshot_blkdev() and monitor command to handle it.
On 12/15/10 17:55, Kevin Wolf wrote:
+int do_snapshot_blkdev(Monitor *mon, const QDict *qdict, QObject **ret_data)
+{
+const char *device = qdict_get_str(qdict, device);
+const char *filename = qdict_get_try_str(qdict, snapshot_file);
+const char *format = qdict_get_try_str(qdict, format);
+const char format_qcow2[] = qcow2;
+BlockDriverState *bs;
+BlockDriver *drv, *proto_drv;
+int ret = 0;
+int flags;
+
+bs = bdrv_find(device);
+if (!bs) {
+qerror_report(QERR_DEVICE_NOT_FOUND, device);
+ret = -1;
+goto out;
+}
+
+if (!format) {
+format = format_qcow2;
+}
+
+drv = bdrv_find_format(format);
+if (!drv) {
+qerror_report(QERR_INVALID_BLOCK_FORMAT, format);
+ret = -1;
+goto out;
+}
+
+proto_drv = bdrv_find_protocol(filename);
+if (!proto_drv) {
+qerror_report(QERR_INVALID_BLOCK_FORMAT, format);
+ret = -1;
+goto out;
+}
+
+ret = bdrv_img_create(filename, format, bs-filename,
+ bs-drv-format_name, NULL, -1, bs-open_flags);
+if (ret) {
+goto out;
+}
+
+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 :(
+ */
+assert(ret == 0);
+out:
+if (ret) {
+ret = 1;
I seem to remember that errors are always -1 for monitor commands.
I mapped it after something else, but admitted I cannot remember where -
can someone clarify?
Cheers,
Jes
[Qemu-devel] Re: [PATCH 0/3] Re-factor img_create() and add live snapshots
On 12/15/10 17:52, Kevin Wolf wrote: Am 13.12.2010 08:32, schrieb jes.soren...@redhat.com: Jes Sorensen (3): qemu-img.c: Re-factor img_create() Introduce do_snapshot_blkdev() and monitor command to handle it. Prevent creating an image with the same filename as backing file This doesn't seem to apply cleanly any more. Can you please rebase (ideally on top of my block branch)? Kevin Darn! I'll try and rebase it tomorrow. Thanks, Jes
Re: [Qemu-devel] KVM call agenda for Dec 14
On 12/14/10 01:12, Chris Wright wrote: Please send in any agenda items you are interested in covering. thanks, -chris Chris, Any chance you could fix your cronjob to send out the CFA a day earlier? 15 hrs before is a bit short notice. Cheers, Jes
[Qemu-devel] Re: [RFC][PATCH v5 08/21] virtagent: add agent_viewfile qmp/hmp command
On 12/10/10 18:09, Michael Roth wrote: I think with strictly enforced size limits the major liability for viewfile is, as you mentioned, users using it to view binary data or carefully crafted files that can mess up or fool users/shells/programs interpreting monitor output. But plain-text does not include escape sequences, so it's completely reasonable that we'd scrape them. And I'm not sure if a (qemu) in the text is a potential liability. Would there be any other issues to consider? If we can guard against those things, do you agree it wouldn't be an inherently dangerous interface? State-full, asynchronous RPCs like copyfile and exec are not really something I'd planned for the initial release. I think they'll take some time to get right, and a simple low-risk interface to cover what I'm fairly sure is the most common use case seems reasonable. I am still wary of relying on strict limit enforcement. It is the sort of thing that will eventually change without us noticing and we end up with a security hole. IMHO QEMU should not try to do these sorts of things, instead it should provide the transport and control services. I don't think file viewing belongs in QEMU at all. I would be a lot more comfortable if this was implemented as a standalone monitor interface that connected to QEMU's QMP interface. I could then use QMP to perform actions like copying the file to /tmp and if viewing the file caused the monitor to lock up, we wouldn't lose the guest. This could indeed be the start of an external monitor :) Cheers, Jes
[Qemu-devel] [PATCH 3/3] Prevent creating an image with the same filename as backing file
From: Jes Sorensen jes.soren...@redhat.com
Signed-off-by: Jes Sorensen jes.soren...@redhat.com
---
block.c |7 +++
1 files changed, 7 insertions(+), 0 deletions(-)
diff --git a/block.c b/block.c
index 3ab062c..403a434 100644
--- a/block.c
+++ b/block.c
@@ -2752,6 +2752,13 @@ int bdrv_img_create(const char *filename, const char
*fmt,
BlockDriver *drv, *proto_drv;
int ret = 0;
+if (!strcmp(filename, base_filename)) {
+error_report(Error: Trying to create a snapshot with the same
+ filename as the backing file);
+ret = -1;
+goto out;
+}
+
/* Find driver and parse its options */
drv = bdrv_find_format(fmt);
if (!drv) {
--
1.7.3.2
[Qemu-devel] [PATCH 0/3] Re-factor img_create() and add live snapshots
From: Jes Sorensen jes.soren...@redhat.com Hi, This set of patches re-factors img_create() and moves the core part of it into block.c so it can be accessed from qemu as well as qemu-img. The second patch adds basic live snapshots support to the code, however only snapshots to external QCOW2 images is supported for now. QED support should be trivial once the QED patches go into upstream. The last patch fixes a small gotcha which is present in the old code as well. Try to catch cases where a user tries to create an image with itself as the backing file. QEMU does 'interesting' things when you do this. Many thanks to Kevin for his help with block layer internals! Cheers, Jes Jes Sorensen (3): qemu-img.c: Re-factor img_create() Introduce do_snapshot_blkdev() and monitor command to handle it. Prevent creating an image with the same filename as backing file block.c | 148 +++ block.h |4 ++ blockdev.c | 61 +++ blockdev.h |1 + hmp-commands.hx | 19 +++ qemu-img.c | 106 +-- 6 files changed, 235 insertions(+), 104 deletions(-) -- 1.7.3.2
[Qemu-devel] [PATCH 2/3] Introduce do_snapshot_blkdev() and monitor command to handle it.
From: Jes Sorensen jes.soren...@redhat.com
The monitor command is:
snapshot_blkdev device [snapshot-file] [format]
Default format is qcow2. For now snapshots without a snapshot-file, eg
internal snapshots, are not supported.
Signed-off-by: Jes Sorensen jes.soren...@redhat.com
---
blockdev.c | 61 +++
blockdev.h |1 +
hmp-commands.hx | 19 +
3 files changed, 81 insertions(+), 0 deletions(-)
diff --git a/blockdev.c b/blockdev.c
index f6ac439..1ea24d7 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -514,6 +514,67 @@ void do_commit(Monitor *mon, const QDict *qdict)
}
}
+int do_snapshot_blkdev(Monitor *mon, const QDict *qdict, QObject **ret_data)
+{
+const char *device = qdict_get_str(qdict, device);
+const char *filename = qdict_get_try_str(qdict, snapshot_file);
+const char *format = qdict_get_try_str(qdict, format);
+const char format_qcow2[] = qcow2;
+BlockDriverState *bs;
+BlockDriver *drv, *proto_drv;
+int ret = 0;
+int flags;
+
+bs = bdrv_find(device);
+if (!bs) {
+qerror_report(QERR_DEVICE_NOT_FOUND, device);
+ret = -1;
+goto out;
+}
+
+if (!format) {
+format = format_qcow2;
+}
+
+drv = bdrv_find_format(format);
+if (!drv) {
+qerror_report(QERR_INVALID_BLOCK_FORMAT, format);
+ret = -1;
+goto out;
+}
+
+proto_drv = bdrv_find_protocol(filename);
+if (!proto_drv) {
+qerror_report(QERR_INVALID_BLOCK_FORMAT, format);
+ret = -1;
+goto out;
+}
+
+ret = bdrv_img_create(filename, format, bs-filename,
+ bs-drv-format_name, NULL, -1, bs-open_flags);
+if (ret) {
+goto out;
+}
+
+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 :(
+ */
+assert(ret == 0);
+out:
+if (ret) {
+ret = 1;
+}
+
+return ret;
+}
+
static int eject_device(Monitor *mon, BlockDriverState *bs, int force)
{
if (!force) {
diff --git a/blockdev.h b/blockdev.h
index 4cb8ca9..4536b5c 100644
--- a/blockdev.h
+++ b/blockdev.h
@@ -52,5 +52,6 @@ int do_block_set_passwd(Monitor *mon, const QDict *qdict,
QObject **ret_data);
int do_change_block(Monitor *mon, const char *device,
const char *filename, const char *fmt);
int do_drive_del(Monitor *mon, const QDict *qdict, QObject **ret_data);
+int do_snapshot_blkdev(Monitor *mon, const QDict *qdict, QObject **ret_data);
#endif
diff --git a/hmp-commands.hx b/hmp-commands.hx
index 23024ba..97e744e 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -801,6 +801,25 @@ STEXI
Set maximum tolerated downtime (in seconds) for migration.
ETEXI
+{
+.name = snapshot_blkdev,
+.args_type = device:s,snapshot_file:s?,format:s?,
+.params = device [snapshot-file] [format],
+.help = initiates a live snapshot\n\t\t\t
+ of device. If a snapshot file is specified, the\n\t\t\t
+ snapshot file will become the new root image.
If\n\t\t\t
+ format is specified, the snapshot file will be\n\t\t\t
+ created in that format. Otherwise the snapshot\n\t\t\t
+ will be internal! (currently unsupported),
+.mhandler.cmd_new = do_snapshot_blkdev,
+},
+
+STEXI
+...@item snapshot_blkdev
+...@findex snapshot_blkdev
+Snapshot device, using snapshot file as target if provided
+ETEXI
+
#if defined(TARGET_I386)
{
.name = drive_add,
--
1.7.3.2
[Qemu-devel] [PATCH 1/3] qemu-img.c: Re-factor img_create()
From: Jes Sorensen jes.soren...@redhat.com
This patch re-factors img_create() moving the code doing the actual
work into block.c where it can be shared with QEMU. This is needed to
be able to create images from QEMU to be used for live snapshots.
Signed-off-by: Jes Sorensen jes.soren...@redhat.com
---
block.c| 141
block.h|4 ++
qemu-img.c | 106 +
3 files changed, 147 insertions(+), 104 deletions(-)
diff --git a/block.c b/block.c
index 63effd8..3ab062c 100644
--- a/block.c
+++ b/block.c
@@ -2742,3 +2742,144 @@ int64_t bdrv_get_dirty_count(BlockDriverState *bs)
{
return bs-dirty_count;
}
+
+int bdrv_img_create(const char *filename, const char *fmt,
+const char *base_filename, const char *base_fmt,
+char *options, uint64_t img_size, int flags)
+{
+QEMUOptionParameter *param = NULL, *create_options = NULL;
+BlockDriverState *bs = NULL;
+BlockDriver *drv, *proto_drv;
+int ret = 0;
+
+/* Find driver and parse its options */
+drv = bdrv_find_format(fmt);
+if (!drv) {
+error_report(Unknown file format '%s', fmt);
+ret = -1;
+goto out;
+}
+
+proto_drv = bdrv_find_protocol(filename);
+if (!proto_drv) {
+error_report(Unknown protocol '%s', filename);
+ret = -1;
+goto out;
+}
+
+create_options = append_option_parameters(create_options,
+ drv-create_options);
+create_options = append_option_parameters(create_options,
+ proto_drv-create_options);
+
+/* Create parameter list with default values */
+param = parse_option_parameters(, create_options, param);
+
+set_option_parameter_int(param, BLOCK_OPT_SIZE, img_size);
+
+/* Parse -o options */
+if (options) {
+param = parse_option_parameters(options, create_options, param);
+if (param == NULL) {
+error_report(Invalid options for file format '%s'., fmt);
+ret = -1;
+goto out;
+}
+}
+
+if (base_filename) {
+if (set_option_parameter(param, BLOCK_OPT_BACKING_FILE,
+ base_filename)) {
+error_report(Backing file not supported for file format '%s',
+ fmt);
+ret = -1;
+goto out;
+}
+}
+if (base_fmt) {
+if (set_option_parameter(param, BLOCK_OPT_BACKING_FMT, base_fmt)) {
+error_report(Backing file format not supported for file
+ format '%s', fmt);
+ret = -1;
+goto out;
+}
+}
+
+// The size for the image must always be specified, with one exception:
+// If we are using a backing file, we can obtain the size from there
+if (get_option_parameter(param, BLOCK_OPT_SIZE)-value.n == -1) {
+QEMUOptionParameter *backing_file =
+get_option_parameter(param, BLOCK_OPT_BACKING_FILE);
+QEMUOptionParameter *backing_fmt =
+get_option_parameter(param, BLOCK_OPT_BACKING_FMT);
+
+if (backing_file backing_file-value.s) {
+uint64_t size;
+const char *fmt = NULL;
+char buf[32];
+
+if (backing_fmt backing_fmt-value.s) {
+if (bdrv_find_format(backing_fmt-value.s)) {
+fmt = backing_fmt-value.s;
+} else {
+error_report(Unknown backing file format '%s',
+ backing_fmt-value.s);
+ret = -1;
+goto out;
+}
+}
+
+bs = bdrv_new();
+if (!bs) {
+error_report(Not enough memory to allocate BlockDriverState);
+ret = -1;
+goto out;
+}
+ret = bdrv_open(bs, backing_file-value.s, flags, drv);
+if (ret 0) {
+error_report(Could not open '%s', filename);
+ret = -1;
+goto out;
+}
+bdrv_get_geometry(bs, size);
+size *= 512;
+
+snprintf(buf, sizeof(buf), % PRId64, size);
+set_option_parameter(param, BLOCK_OPT_SIZE, buf);
+} else {
+error_report(Image creation needs a size parameter);
+ret = -1;
+goto out;
+}
+}
+
+printf(Formatting '%s', fmt=%s , filename, fmt);
+print_option_parameters(param);
+puts();
+
+ret = bdrv_create(drv, filename, param);
+free_option_parameters(create_options);
+free_option_parameters(param);
+
+if (ret 0) {
+if (ret == -ENOTSUP) {
+error_report(Formatting or formatting option not supported for
+ file format '%s', fmt);
+} else if (ret == -EFBIG
[Qemu-devel] Re: [RFC][PATCH v5 07/21] virtagent: add va.getfile RPC
On 12/09/10 22:04, Michael Roth wrote: On 12/09/2010 08:40 AM, Adam Litke wrote: Actually, a host-controlled interface would be both simpler to implement (on both ends) and would concentrate control in the host (which is what we probably want anyway). I also like the host-controlled mechanism. I think the difficulty is about the same either way though. Both would require an FD to be kept open, and some mechanism to read/seek in chunks and then send it back. I don't think this should replace the underlying RPC for viewfile however. I'd much rather it be an additional RPC, and we just put strict limits on the size of files viewfile/getfile can handle and make it clear that it is intended for quickly viewing text. I'll rename the getfile RPC to viewfile to make this a bit clearer as well. I really think the viewfile stuff needs to go away, it is a rats trap for things that can go wrong. It would really only be useful from the human monitor, whereas any other application will be happy to implement it at the higher level. Having it in the human monitor you risk messing it up by viewing a binary file and you end up having to kill you guest to fix it. Putting artificial limits on it means someone will eventually try to change those. Lets keep this at the higher level, where it IMHO belongs. Cheers, Jes
Re: [Qemu-devel] Re: [RFC][PATCH v5 09/21] virtagent: add va.getdmesg RPC
On 12/07/10 18:32, Michael Roth wrote:
On 12/07/2010 08:37 AM, Jes Sorensen wrote:
On 12/03/10 19:03, Michael Roth wrote:
+static xmlrpc_value *va_getdmesg(xmlrpc_env *env,
+ xmlrpc_value *param,
+ void *user_data)
+{
+char *dmesg_buf = NULL, cmd[256];
+int ret;
+xmlrpc_value *result = NULL;
+FILE *pipe;
+
+SLOG(va_getdmesg());
+
+dmesg_buf = qemu_mallocz(VA_DMESG_LEN + 2048);
+sprintf(cmd, dmesg -s %d, VA_DMESG_LEN);
What happens if the guest's dmesg buffer is larger than your hardcoded
value?
It'll end up getting truncated by the fread() later:
ret = fread(dmesg_buf, sizeof(char), VA_DMESG_LEN, pipe);
That's where the dmesg -s VA_DMESG_LEN comes into play, it should size
things such that we can buffer up till the end of the dmesg output.
This param is kind of quirky though, size doesn't seem to have an affect
for anything below 4KB, but if we stick with VA_DMESG_LEN = 4KB this
should cover us, unless it's a distro-specific. But it should blow
anything up, at least.
I am wary of these hard coded constants. Isn't there a way to set the
kernel's dmesg buffer size, or is that only a compile time option?
Cheers,
Jes
Re: [Qemu-devel] Re: [RFC][PATCH v5 04/21] virtagent: transport definitions and job callbacks
On 12/07/10 18:19, Michael Roth wrote:
On 12/07/2010 07:44 AM, Jes Sorensen wrote:
+static int va_end_of_header(char *buf, int end_pos)
+{
+return !strncmp(buf+(end_pos-2), \n\r\n, 3);
+}
Maybe I am missing something here, but it looks like you do a strncmp to
a char that is one past the end of the buffer, or? If this is
intentional, please document it.
buf+end_pos points to the last char we read (rather than being an offset
to the current position). So it stops comparing when it reaches
buf+end_pos (buf=0 + end_pos=2 implies 3 characters)
For some reason this confused the hell out of me when I looked over it
again as well. Alternatively I can do:
static int va_end_of_header(char *buf, int end_pos)
{
return !strncmp(buf+(end_pos-2), \n\r\n, 3);
}
...
va_end_of_header(s-hdr, s-hdr_pos - 1)
-
static int va_end_of_header(char *buf, int cur_pos)
{
return !strncmp(buf+(cur_pos-3), \n\r\n, 3);
}
...
va_end_of_header(s-hdr, s-hdr_pos);
It does seem easier to parse...
I would prefer this, somewhat easier to parse.
All this http parsing code leaves the question open why you do it
manually, instead of relying on a library?
Something like libcurl? At some point we didn't attempt to use libraries
provide by xmlrpc-c (which uses libcurl for http transport) for the
client and server. The problem there is that libcurl really wants and
tcp socket read and write from, whereas we need to support tcp/unix
sockets on the host side and isa/virtio serial ports on the guest side.
Even assuming we could hook in wrappers for these other types of
sockets/channels, there's also the added complexity since dropping
virtproxy of multiplexing HTTP/RPCs using a single stream, whereas
something like libcurl would, understandably, assume it has a dedicated
stream to read/write from. So we wouldn't really save any work or code,
unfortunately.
I guess I am just a little worried that we end up with errors in the
code that could have been solved by using a maintainer http library, but
if it isn't feasible I guess not.
Cheers,
Jes
[Qemu-devel] Re: [RFC][PATCH v5 07/21] virtagent: add va.getfile RPC
On 12/07/10 17:00, Adam Litke wrote: Hi Jes, you raise some good points and pitfalls with the current getfile approach. I've been thinking about an alternative and am wondering what you (and others) think... First off, I think we should switch to a copyfile() API that allows us to avoid presenting the file contents to the user. Neither the human monitor nor the control monitor are designed to be file pagers. Let the user decide how to consume the data once it has been transferred. Now we don't need to care if the file is binary or text. The virtagent RPC protocol is bi-directional and supports asynchronous events. We can use these to implement a better copyfile RPC that can transfer larger files without wasting memory. The host issues a copyfile(guest-path, host-path) RPC. The immediate result of this call will indicate whether the guest is able to initiate the transfer. The guest will generate a series of events (offset, size, payload) until the entire contents has been transferred. The host and guest could negotiate the chunk size if necessary. Once the transfer is complete, the guest sends a final event to indicate this (file-size, 0). This interface could be integrated into the monitor with a pair of commands (va_copyfile and info va_copyfile), the former used to initiate transfers and the latter to check on the status. Thoughts on this? Hi Adam, This sounds a lot safer than the current approach. Intuitively I would think it should be the host controlling the copy, but otherwise it sounds good. Or is there a reason why the guest should control it? I think it is vital that we do it in a way where a copy cannot blow QEMU's memory consumption out of the water, but the approach you suggest seems to take care of that. Cheers, Jes
[Qemu-devel] [PATCH 2/2] Make img_create() use strtosz_suffix()
From: Jes Sorensen jes.soren...@redhat.com
This reestablished the old default of using bytes as the default for
the size argument, and not MB as we do in pretty much every other
place.
Signed-off-by: Jes Sorensen jes.soren...@redhat.com
---
qemu-img.c |2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/qemu-img.c b/qemu-img.c
index 9a5e7e1..603bdb3 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -331,7 +331,7 @@ static int img_create(int argc, char **argv)
/* Get image size, if specified */
if (optind argc) {
ssize_t sval;
-sval = strtosz(argv[optind++], NULL);
+sval = strtosz_suffix(argv[optind++], NULL, STRTOSZ_DEFSUFFIX_B);
if (sval 0) {
error(Invalid image size specified! You may use k, M, G or
T suffixes for );
--
1.7.3.2
[Qemu-devel] [PATCH v3 1/1] qemu-img.c: Clean up handling of image size in img_create()
From: Jes Sorensen jes.soren...@redhat.com
This cleans up the handling of image size in img_create() by parsing
the value early, and then only setting it once if a value has been
added as the last argument to the command line.
Signed-off-by: Jes Sorensen jes.soren...@redhat.com
---
qemu-img.c | 23 +--
1 files changed, 17 insertions(+), 6 deletions(-)
diff --git a/qemu-img.c b/qemu-img.c
index d146d8c..d9667a2 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -282,6 +282,7 @@ static int add_old_style_options(const char *fmt,
QEMUOptionParameter *list,
static int img_create(int argc, char **argv)
{
int c, ret = 0;
+uint64_t img_size = -1;
const char *fmt = raw;
const char *base_fmt = NULL;
const char *filename;
@@ -329,6 +330,20 @@ static int img_create(int argc, char **argv)
}
filename = argv[optind++];
+/* Get image size, if specified */
+if (optind argc) {
+ssize_t sval;
+sval = strtosz(argv[optind++], NULL);
+if (sval 0) {
+error(Invalid image size specified! You may use k, M, G or
+ T suffixes for );
+error(kilobytes, megabytes, gigabytes and terabytes.);
+ret = -1;
+goto out;
+}
+img_size = (uint64_t)sval;
+}
+
if (options !strcmp(options, ?)) {
ret = print_block_option_help(filename, fmt);
goto out;
@@ -356,7 +371,8 @@ static int img_create(int argc, char **argv)
/* Create parameter list with default values */
param = parse_option_parameters(, create_options, param);
-set_option_parameter_int(param, BLOCK_OPT_SIZE, -1);
+
+set_option_parameter_int(param, BLOCK_OPT_SIZE, img_size);
/* Parse -o options */
if (options) {
@@ -368,11 +384,6 @@ static int img_create(int argc, char **argv)
}
}
-/* Add size to parameters */
-if (optind argc) {
-set_option_parameter(param, BLOCK_OPT_SIZE, argv[optind++]);
-}
-
/* Add old-style options to parameters */
ret = add_old_style_options(fmt, param, base_filename, base_fmt);
if (ret 0) {
--
1.7.3.2
[Qemu-devel] [PATCH 1/2] Introduce strtosz_suffix()
From: Jes Sorensen jes.soren...@redhat.com
This introduces strtosz_suffix() which allows the caller to specify a
default suffix in case the non default of MB is wanted.
strtosz() is kept as a wrapper for strtosz_suffix() which keeps it's
current default of MB.
Signed-off-by: Jes Sorensen jes.soren...@redhat.com
---
cutils.c | 17 ++---
qemu-common.h |7 +++
2 files changed, 21 insertions(+), 3 deletions(-)
diff --git a/cutils.c b/cutils.c
index 28089aa..1d24d9a 100644
--- a/cutils.c
+++ b/cutils.c
@@ -291,10 +291,10 @@ int fcntl_setfl(int fd, int flag)
* value must be terminated by whitespace, ',' or '\0'. Return -1 on
* error.
*/
-ssize_t strtosz(const char *nptr, char **end)
+ssize_t strtosz_suffix(const char *nptr, char **end, const char default_suffix)
{
ssize_t retval = -1;
-char *endptr, c;
+char *endptr, c, d;
int mul_required = 0;
double val, mul, integral, fraction;
@@ -313,10 +313,16 @@ ssize_t strtosz(const char *nptr, char **end)
* part of a multi token argument.
*/
c = *endptr;
+d = c;
if (isspace(c) || c == '\0' || c == ',') {
c = 0;
+if (default_suffix) {
+d = default_suffix;
+} else {
+d = c;
+}
}
-switch (c) {
+switch (d) {
case 'B':
case 'b':
mul = 1;
@@ -371,3 +377,8 @@ fail:
return retval;
}
+
+ssize_t strtosz(const char *nptr, char **end)
+{
+return strtosz_suffix(nptr, end, 0);
+}
diff --git a/qemu-common.h b/qemu-common.h
index de82c2e..dc44cd6 100644
--- a/qemu-common.h
+++ b/qemu-common.h
@@ -149,7 +149,14 @@ time_t mktimegm(struct tm *tm);
int qemu_fls(int i);
int qemu_fdatasync(int fd);
int fcntl_setfl(int fd, int flag);
+
+#define STRTOSZ_DEFSUFFIX_TB 'T'
+#define STRTOSZ_DEFSUFFIX_GB 'G'
+#define STRTOSZ_DEFSUFFIX_MB 'M'
+#define STRTOSZ_DEFSUFFIX_KB 'K'
+#define STRTOSZ_DEFSUFFIX_B'B'
ssize_t strtosz(const char *nptr, char **end);
+ssize_t strtosz_suffix(const char *nptr, char **end, const char);
/* path.c */
void init_paths(const char *prefix);
--
1.7.3.2
[Qemu-devel] [PATCH 1/2] Introduce strtosz_suffix()
From: Jes Sorensen jes.soren...@redhat.com
This introduces strtosz_suffix() which allows the caller to specify a
default suffix in case the non default of MB is wanted.
strtosz() is kept as a wrapper for strtosz_suffix() which keeps it's
current default of MB.
Signed-off-by: Jes Sorensen jes.soren...@redhat.com
---
cutils.c | 17 ++---
qemu-common.h |7 +++
2 files changed, 21 insertions(+), 3 deletions(-)
diff --git a/cutils.c b/cutils.c
index 28089aa..1d24d9a 100644
--- a/cutils.c
+++ b/cutils.c
@@ -291,10 +291,10 @@ int fcntl_setfl(int fd, int flag)
* value must be terminated by whitespace, ',' or '\0'. Return -1 on
* error.
*/
-ssize_t strtosz(const char *nptr, char **end)
+ssize_t strtosz_suffix(const char *nptr, char **end, const char default_suffix)
{
ssize_t retval = -1;
-char *endptr, c;
+char *endptr, c, d;
int mul_required = 0;
double val, mul, integral, fraction;
@@ -313,10 +313,16 @@ ssize_t strtosz(const char *nptr, char **end)
* part of a multi token argument.
*/
c = *endptr;
+d = c;
if (isspace(c) || c == '\0' || c == ',') {
c = 0;
+if (default_suffix) {
+d = default_suffix;
+} else {
+d = c;
+}
}
-switch (c) {
+switch (d) {
case 'B':
case 'b':
mul = 1;
@@ -371,3 +377,8 @@ fail:
return retval;
}
+
+ssize_t strtosz(const char *nptr, char **end)
+{
+return strtosz_suffix(nptr, end, 0);
+}
diff --git a/qemu-common.h b/qemu-common.h
index de82c2e..dc44cd6 100644
--- a/qemu-common.h
+++ b/qemu-common.h
@@ -149,7 +149,14 @@ time_t mktimegm(struct tm *tm);
int qemu_fls(int i);
int qemu_fdatasync(int fd);
int fcntl_setfl(int fd, int flag);
+
+#define STRTOSZ_DEFSUFFIX_TB 'T'
+#define STRTOSZ_DEFSUFFIX_GB 'G'
+#define STRTOSZ_DEFSUFFIX_MB 'M'
+#define STRTOSZ_DEFSUFFIX_KB 'K'
+#define STRTOSZ_DEFSUFFIX_B'B'
ssize_t strtosz(const char *nptr, char **end);
+ssize_t strtosz_suffix(const char *nptr, char **end, const char);
/* path.c */
void init_paths(const char *prefix);
--
1.7.3.2
[Qemu-devel] [PATCH v4 0/2] Clean up img_create() and introduce strtosz_suffix()
From: Jes Sorensen jes.soren...@redhat.com This patch set introduces strtosz_suffix() which is needed to be able to use strtosz parsing with a non MB default suffix. This is used to clean up qemu-img.c:img_create(). Kevin asked me to rebase this instead of applying the other patches on top, so please discard the previous versions. Sorry for the patch noise. Jes Sorensen (2): Introduce strtosz_suffix() qemu-img.c: Clean up handling of image size in img_create() cutils.c | 17 ++--- qemu-common.h |7 +++ qemu-img.c| 23 +-- 3 files changed, 38 insertions(+), 9 deletions(-) -- 1.7.3.2
[Qemu-devel] [PATCH 2/2] qemu-img.c: Clean up handling of image size in img_create()
From: Jes Sorensen jes.soren...@redhat.com
This cleans up the handling of image size in img_create() by parsing
the value early, and then only setting it once if a value has been
added as the last argument to the command line.
Signed-off-by: Jes Sorensen jes.soren...@redhat.com
---
qemu-img.c | 23 +--
1 files changed, 17 insertions(+), 6 deletions(-)
diff --git a/qemu-img.c b/qemu-img.c
index d146d8c..f078718 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -282,6 +282,7 @@ static int add_old_style_options(const char *fmt,
QEMUOptionParameter *list,
static int img_create(int argc, char **argv)
{
int c, ret = 0;
+uint64_t img_size = -1;
const char *fmt = raw;
const char *base_fmt = NULL;
const char *filename;
@@ -329,6 +330,20 @@ static int img_create(int argc, char **argv)
}
filename = argv[optind++];
+/* Get image size, if specified */
+if (optind argc) {
+ssize_t sval;
+sval = strtosz_suffix(argv[optind++], NULL, STRTOSZ_DEFSUFFIX_B);
+if (sval 0) {
+error(Invalid image size specified! You may use k, M, G or
+ T suffixes for );
+error(kilobytes, megabytes, gigabytes and terabytes.);
+ret = -1;
+goto out;
+}
+img_size = (uint64_t)sval;
+}
+
if (options !strcmp(options, ?)) {
ret = print_block_option_help(filename, fmt);
goto out;
@@ -356,7 +371,8 @@ static int img_create(int argc, char **argv)
/* Create parameter list with default values */
param = parse_option_parameters(, create_options, param);
-set_option_parameter_int(param, BLOCK_OPT_SIZE, -1);
+
+set_option_parameter_int(param, BLOCK_OPT_SIZE, img_size);
/* Parse -o options */
if (options) {
@@ -368,11 +384,6 @@ static int img_create(int argc, char **argv)
}
}
-/* Add size to parameters */
-if (optind argc) {
-set_option_parameter(param, BLOCK_OPT_SIZE, argv[optind++]);
-}
-
/* Add old-style options to parameters */
ret = add_old_style_options(fmt, param, base_filename, base_fmt);
if (ret 0) {
--
1.7.3.2
[Qemu-devel] [PATCH 0/2] Fix size default for qemu-img
From: Jes Sorensen jes.soren...@redhat.com Kevin pointed out that my chance to img_create()'s handling of the image size, changed the previous default of byte for size if no suffix was specified, since strtosz() defaults to MB. This patch set introduces strtosz_suffix() and then changes img_create() to use that instead, thereby restoring the old default behavior. Jes Sorensen (2): Introduce strtosz_suffix() Make img_create() use strtosz_suffix() cutils.c | 17 ++--- qemu-common.h |7 +++ qemu-img.c|2 +- 3 files changed, 22 insertions(+), 4 deletions(-) -- 1.7.3.2
[Qemu-devel] [PATCH 1/1] Introduce strtosz_suffix()
From: Jes Sorensen jes.soren...@redhat.com
This introduces strtosz_suffix() which allows the caller to specify a
default suffix in case the non default of MB is wanted.
strtosz() is kept as a wrapper for strtosz_suffix() which keeps it's
current default of MB.
Signed-off-by: Jes Sorensen jes.soren...@redhat.com
---
cutils.c | 17 ++---
qemu-common.h |7 +++
2 files changed, 21 insertions(+), 3 deletions(-)
diff --git a/cutils.c b/cutils.c
index 28089aa..1d24d9a 100644
--- a/cutils.c
+++ b/cutils.c
@@ -291,10 +291,10 @@ int fcntl_setfl(int fd, int flag)
* value must be terminated by whitespace, ',' or '\0'. Return -1 on
* error.
*/
-ssize_t strtosz(const char *nptr, char **end)
+ssize_t strtosz_suffix(const char *nptr, char **end, const char default_suffix)
{
ssize_t retval = -1;
-char *endptr, c;
+char *endptr, c, d;
int mul_required = 0;
double val, mul, integral, fraction;
@@ -313,10 +313,16 @@ ssize_t strtosz(const char *nptr, char **end)
* part of a multi token argument.
*/
c = *endptr;
+d = c;
if (isspace(c) || c == '\0' || c == ',') {
c = 0;
+if (default_suffix) {
+d = default_suffix;
+} else {
+d = c;
+}
}
-switch (c) {
+switch (d) {
case 'B':
case 'b':
mul = 1;
@@ -371,3 +377,8 @@ fail:
return retval;
}
+
+ssize_t strtosz(const char *nptr, char **end)
+{
+return strtosz_suffix(nptr, end, 0);
+}
diff --git a/qemu-common.h b/qemu-common.h
index de82c2e..dc44cd6 100644
--- a/qemu-common.h
+++ b/qemu-common.h
@@ -149,7 +149,14 @@ time_t mktimegm(struct tm *tm);
int qemu_fls(int i);
int qemu_fdatasync(int fd);
int fcntl_setfl(int fd, int flag);
+
+#define STRTOSZ_DEFSUFFIX_TB 'T'
+#define STRTOSZ_DEFSUFFIX_GB 'G'
+#define STRTOSZ_DEFSUFFIX_MB 'M'
+#define STRTOSZ_DEFSUFFIX_KB 'K'
+#define STRTOSZ_DEFSUFFIX_B'B'
ssize_t strtosz(const char *nptr, char **end);
+ssize_t strtosz_suffix(const char *nptr, char **end, const char);
/* path.c */
void init_paths(const char *prefix);
--
1.7.3.2
[Qemu-devel] [PATCH 2/2] qemu-img.c: Clean up handling of image size in img_create()
From: Jes Sorensen jes.soren...@redhat.com
This cleans up the handling of image size in img_create() by parsing
the value early, and then only setting it once if a value has been
added as the last argument to the command line.
Signed-off-by: Jes Sorensen jes.soren...@redhat.com
---
qemu-img.c | 23 +--
1 files changed, 17 insertions(+), 6 deletions(-)
diff --git a/qemu-img.c b/qemu-img.c
index d146d8c..f078718 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -282,6 +282,7 @@ static int add_old_style_options(const char *fmt,
QEMUOptionParameter *list,
static int img_create(int argc, char **argv)
{
int c, ret = 0;
+uint64_t img_size = -1;
const char *fmt = raw;
const char *base_fmt = NULL;
const char *filename;
@@ -329,6 +330,20 @@ static int img_create(int argc, char **argv)
}
filename = argv[optind++];
+/* Get image size, if specified */
+if (optind argc) {
+ssize_t sval;
+sval = strtosz_suffix(argv[optind++], NULL, STRTOSZ_DEFSUFFIX_B);
+if (sval 0) {
+error(Invalid image size specified! You may use k, M, G or
+ T suffixes for );
+error(kilobytes, megabytes, gigabytes and terabytes.);
+ret = -1;
+goto out;
+}
+img_size = (uint64_t)sval;
+}
+
if (options !strcmp(options, ?)) {
ret = print_block_option_help(filename, fmt);
goto out;
@@ -356,7 +371,8 @@ static int img_create(int argc, char **argv)
/* Create parameter list with default values */
param = parse_option_parameters(, create_options, param);
-set_option_parameter_int(param, BLOCK_OPT_SIZE, -1);
+
+set_option_parameter_int(param, BLOCK_OPT_SIZE, img_size);
/* Parse -o options */
if (options) {
@@ -368,11 +384,6 @@ static int img_create(int argc, char **argv)
}
}
-/* Add size to parameters */
-if (optind argc) {
-set_option_parameter(param, BLOCK_OPT_SIZE, argv[optind++]);
-}
-
/* Add old-style options to parameters */
ret = add_old_style_options(fmt, param, base_filename, base_fmt);
if (ret 0) {
--
1.7.3.2
[Qemu-devel] [PATCH v5 0/2] Clean up img_create() and introduce strtosz_suffix()
From: Jes Sorensen jes.soren...@redhat.com This patch set introduces strtosz_suffix() which is needed to be able to use strtosz parsing with a non MB default suffix. This is used to clean up qemu-img.c:img_create(). Kevin asked me to rebase this instead of applying the other patches on top, so please discard the previous versions. Sorry for the patch noise. v5 fixes the two issues pointed out by Stefan, making the call in strtosz() explicitly use STRTOSZ_DEFSUFFIX_MB instead of 0 to specify the default and adds a named argument to the prototype for strtosz_suffix(). Jes Sorensen (2): Introduce strtosz_suffix() qemu-img.c: Clean up handling of image size in img_create() cutils.c | 17 ++--- qemu-common.h |7 +++ qemu-img.c| 23 +-- 3 files changed, 38 insertions(+), 9 deletions(-) -- 1.7.3.2
[Qemu-devel] Re: [PATCH 1/2] Introduce strtosz_suffix()
On 12/09/10 13:53, Stefan Hajnoczi wrote:
On Thu, Dec 09, 2010 at 01:13:33PM +0100, jes.soren...@redhat.com wrote:
@@ -371,3 +377,8 @@ fail:
return retval;
}
+
+ssize_t strtosz(const char *nptr, char **end)
+{
+return strtosz_suffix(nptr, end, 0);
This obscures what the default is, please use STRTOSZ_DEFSUFFIX_MB. 0
isn't very meaningful and requires the reader to dig into strtosz().
It doesn't make much different to me, but ok, that is easy to fix.
diff --git a/qemu-common.h b/qemu-common.h
index de82c2e..dc44cd6 100644
--- a/qemu-common.h
+++ b/qemu-common.h
@@ -149,7 +149,14 @@ time_t mktimegm(struct tm *tm);
int qemu_fls(int i);
int qemu_fdatasync(int fd);
int fcntl_setfl(int fd, int flag);
+
+#define STRTOSZ_DEFSUFFIX_TB'T'
+#define STRTOSZ_DEFSUFFIX_GB'G'
+#define STRTOSZ_DEFSUFFIX_MB'M'
+#define STRTOSZ_DEFSUFFIX_KB'K'
+#define STRTOSZ_DEFSUFFIX_B 'B'
ssize_t strtosz(const char *nptr, char **end);
+ssize_t strtosz_suffix(const char *nptr, char **end, const char);
An argument name would be nice: const char unit?
I'll add that in the same round.
Cheers,
Jes
[Qemu-devel] [PATCH 1/2] Introduce strtosz_suffix()
From: Jes Sorensen jes.soren...@redhat.com
This introduces strtosz_suffix() which allows the caller to specify a
default suffix in case the non default of MB is wanted.
strtosz() is kept as a wrapper for strtosz_suffix() which keeps it's
current default of MB.
Signed-off-by: Jes Sorensen jes.soren...@redhat.com
---
cutils.c | 17 ++---
qemu-common.h |7 +++
2 files changed, 21 insertions(+), 3 deletions(-)
diff --git a/cutils.c b/cutils.c
index 28089aa..7984bc1 100644
--- a/cutils.c
+++ b/cutils.c
@@ -291,10 +291,10 @@ int fcntl_setfl(int fd, int flag)
* value must be terminated by whitespace, ',' or '\0'. Return -1 on
* error.
*/
-ssize_t strtosz(const char *nptr, char **end)
+ssize_t strtosz_suffix(const char *nptr, char **end, const char default_suffix)
{
ssize_t retval = -1;
-char *endptr, c;
+char *endptr, c, d;
int mul_required = 0;
double val, mul, integral, fraction;
@@ -313,10 +313,16 @@ ssize_t strtosz(const char *nptr, char **end)
* part of a multi token argument.
*/
c = *endptr;
+d = c;
if (isspace(c) || c == '\0' || c == ',') {
c = 0;
+if (default_suffix) {
+d = default_suffix;
+} else {
+d = c;
+}
}
-switch (c) {
+switch (d) {
case 'B':
case 'b':
mul = 1;
@@ -371,3 +377,8 @@ fail:
return retval;
}
+
+ssize_t strtosz(const char *nptr, char **end)
+{
+return strtosz_suffix(nptr, end, STRTOSZ_DEFSUFFIX_MB);
+}
diff --git a/qemu-common.h b/qemu-common.h
index de82c2e..1ed32e5 100644
--- a/qemu-common.h
+++ b/qemu-common.h
@@ -149,7 +149,14 @@ time_t mktimegm(struct tm *tm);
int qemu_fls(int i);
int qemu_fdatasync(int fd);
int fcntl_setfl(int fd, int flag);
+
+#define STRTOSZ_DEFSUFFIX_TB 'T'
+#define STRTOSZ_DEFSUFFIX_GB 'G'
+#define STRTOSZ_DEFSUFFIX_MB 'M'
+#define STRTOSZ_DEFSUFFIX_KB 'K'
+#define STRTOSZ_DEFSUFFIX_B'B'
ssize_t strtosz(const char *nptr, char **end);
+ssize_t strtosz_suffix(const char *nptr, char **end, const char
default_suffix);
/* path.c */
void init_paths(const char *prefix);
--
1.7.3.2
[Qemu-devel] Re: [RFC][PATCH v5 08/21] virtagent: add agent_viewfile qmp/hmp command
On 12/09/10 22:12, Michael Roth wrote:
On 12/07/2010 08:26 AM, Jes Sorensen wrote:
I believe this suffers from the same architectural problem I mentioned
in my comment to 07/21 - you don't restrict the file size, so it could
blow up the QEMU process on the host trying to view the wrong file.
It's restricted on the guest side:
virtagent-server.c:va_getfile():
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);
count += ret;
if (count VA_GETFILE_MAX) {
xmlrpc_faultf(env, max file size (%d bytes) exceeded,
VA_GETFILE_MAX);
goto EXIT_CLOSE_BAD;
}
}
You cannot rely on the guest controlling this. You really have to treat
any guest as hostile and keep control and security in the host,
otherwise a hacked guest could end up attacking the host by blowing up
the host's QEMU process.
Cheers,
Jes
Re: [Qemu-devel] Re: [RFC][PATCH v5 09/21] virtagent: add va.getdmesg RPC
On 12/09/10 22:15, Michael Roth wrote: On 12/08/2010 01:22 PM, Jes Sorensen wrote: This param is kind of quirky though, size doesn't seem to have an affect for anything below 4KB, but if we stick with VA_DMESG_LEN= 4KB this should cover us, unless it's a distro-specific. But it should blow anything up, at least. I am wary of these hard coded constants. Isn't there a way to set the kernel's dmesg buffer size, or is that only a compile time option? From what I can tell it's a compile-time option. I originally had dmesg_len as a param the host could pass to the guest, but it has no effect if the buffer is smaller, which might cause unnecessary confusion. You are correct! I checked, there were people talking about a configurable option but so far it is not in place. I would still rather have this go via a file transfer to the host, and put the output in a tmpfile. I am still against adding viewfile commands to the monitor though. You get a bad control character in the string and your console is messed up. Hit CTRL-c and you lost your guest. Cheers, Jes
[Qemu-devel] [PATCH v2 1/1] qemu-img.c: Clean up handling of image size in img_create()
From: Jes Sorensen jes.soren...@redhat.com
This cleans up the handling of image size in img_create() by parsing
the value early, and then only setting it once if a value has been
added as the last argument to the command line.
Signed-off-by: Jes Sorensen jes.soren...@redhat.com
---
qemu-img.c | 14 --
1 files changed, 8 insertions(+), 6 deletions(-)
diff --git a/qemu-img.c b/qemu-img.c
index d146d8c..9986004 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -282,6 +282,7 @@ static int add_old_style_options(const char *fmt,
QEMUOptionParameter *list,
static int img_create(int argc, char **argv)
{
int c, ret = 0;
+uint64_t img_size = -1;
const char *fmt = raw;
const char *base_fmt = NULL;
const char *filename;
@@ -329,6 +330,11 @@ static int img_create(int argc, char **argv)
}
filename = argv[optind++];
+/* Get image size, if specified */
+if (optind argc) {
+img_size = strtosz(argv[optind++], NULL);
+}
+
if (options !strcmp(options, ?)) {
ret = print_block_option_help(filename, fmt);
goto out;
@@ -356,7 +362,8 @@ static int img_create(int argc, char **argv)
/* Create parameter list with default values */
param = parse_option_parameters(, create_options, param);
-set_option_parameter_int(param, BLOCK_OPT_SIZE, -1);
+
+set_option_parameter_int(param, BLOCK_OPT_SIZE, img_size);
/* Parse -o options */
if (options) {
@@ -368,11 +375,6 @@ static int img_create(int argc, char **argv)
}
}
-/* Add size to parameters */
-if (optind argc) {
-set_option_parameter(param, BLOCK_OPT_SIZE, argv[optind++]);
-}
-
/* Add old-style options to parameters */
ret = add_old_style_options(fmt, param, base_filename, base_fmt);
if (ret 0) {
--
1.7.3.2
Re: [Qemu-devel] [PATCH 1/1] qemu-img.c: Clean up handling of image size in img_create()
On 12/08/10 09:54, Kevin Wolf wrote: Am 07.12.2010 21:36, schrieb Stefan Hajnoczi: Today it is possible to create 0 byte sized images. Your patch will change that: If there is a backing file, then the size will be taken from the backing file. If there is no backing file, then an error about missing size will be printed, even though a size of 0 has been given. I can think of one use case for it: You can store the VM state on a zero-sized qcow2 image for internal snapshots. Otherwise it's probably rather useless, but we have supported it for a long time, so I wouldn't remove it. People have actually noticed in the past when something was broken with it. Ok that is fair, I have just posted an updated version which should do the right thing. Shows up it made the patch even simpler :) Cheers, Jes
Re: [Qemu-devel] Re: [RFC][PATCH v5 01/21] Move code related to fd handlers into utility functions
On 12/08/10 10:15, Stefan Hajnoczi wrote: On Tue, Dec 07, 2010 at 04:02:03PM +0100, Jes Sorensen wrote: Anything to avoid qemu_set_fd_handler17() at some point. I think using __qemu_set_fd_handler() encourages people to modify that code rather than copy it. I agree that qemu_set_fd_handler3() could be named something more meaningful. Unfortunately we can't use __qemu_set_fd_handler() because the C language standard reserves identifiers that start with double underscore for the standard library. The Linux kernel gets away with it because the code is freestanding but that doesn't apply to QEMU. Hmmm you sure that is actually a rule, rather than a convention? Either way, we can use 3 underscores, or leave the function static, in which case the C library names are a non issue. Cheers, Jes
Re: [Qemu-devel] Re: [RFC][PATCH v5 01/21] Move code related to fd handlers into utility functions
On 12/08/10 10:23, Stefan Hajnoczi wrote: From 7.1.3 Reserved identifiers: All identifiers that begin with an underscore and either an uppercase letter or another underscore are always reserved for any use. and All identifiers that begin with an underscore are always reserved for use as identifiers with file scope in both the ordinary and tag name spaces. That includes three or more underscores too. Ok, I never hit problems with this, but ok we can name it do_qemu_set_fd_handler() instead. That would go with the existing naming conventions used in many places throughout the code. Cheers, Jes
[Qemu-devel] Re: [RFC][PATCH v5 00/21] virtagent: host/guest RPC communication agent
On 12/03/10 19:03, Michael Roth wrote: These patches apply to master, and can also be obtained from: git://repo.or.cz/qemu/mdroth.git virtagent_v5 CHANGES IN V5: - Dependency on virtproxy dropped, virtagent now handles transport and multiplexing of bi-directional RPCs internally - Removed duplification of qemu_set_fd_handler()-centered i/o code. Support for interacting with objects that use qemu_set_fd_handler() now available to tools via qemu-tools.c and a set of generalized utility functions - Fixed memory leaks in client/monitor functions - Various cleanups Hi Michael, Does this mean that virtproxy is now obsolete, or does it just mean using virtproxy is optional? I would still prefer to have virtagent a separate package, rather than part of the QEMU tree though. Thanks, Jes
[Qemu-devel] [PATCH 1/1] qemu-img: Deprecate obsolete -6 and -e options
From: Jes Sorensen jes.soren...@redhat.com
If -6 or -e is specified, an error message is printed and we exit. It
does not print help() to avoid the error message getting lost in the
noise.
Signed-off-by: Jes Sorensen jes.soren...@redhat.com
---
block_int.h |1 -
qemu-img.c | 53 ++---
2 files changed, 22 insertions(+), 32 deletions(-)
diff --git a/block_int.h b/block_int.h
index 3c3adb5..3ceed47 100644
--- a/block_int.h
+++ b/block_int.h
@@ -29,7 +29,6 @@
#include qemu-queue.h
#define BLOCK_FLAG_ENCRYPT 1
-#define BLOCK_FLAG_COMPRESS2
#define BLOCK_FLAG_COMPAT6 4
#define BLOCK_OPT_SIZE size
diff --git a/qemu-img.c b/qemu-img.c
index 5b6e648..16fec40 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -261,21 +261,9 @@ fail:
}
static int add_old_style_options(const char *fmt, QEMUOptionParameter *list,
-int flags, const char *base_filename, const char *base_fmt)
+ const char *base_filename,
+ const char *base_fmt)
{
-if (flags BLOCK_FLAG_ENCRYPT) {
-if (set_option_parameter(list, BLOCK_OPT_ENCRYPT, on)) {
-error(Encryption not supported for file format '%s', fmt);
-return -1;
-}
-}
-if (flags BLOCK_FLAG_COMPAT6) {
-if (set_option_parameter(list, BLOCK_OPT_COMPAT6, on)) {
-error(VMDK version 6 not supported for file format '%s', fmt);
-return -1;
-}
-}
-
if (base_filename) {
if (set_option_parameter(list, BLOCK_OPT_BACKING_FILE, base_filename))
{
error(Backing file not supported for file format '%s', fmt);
@@ -293,7 +281,7 @@ static int add_old_style_options(const char *fmt,
QEMUOptionParameter *list,
static int img_create(int argc, char **argv)
{
-int c, ret = 0, flags;
+int c, ret = 0;
const char *fmt = raw;
const char *base_fmt = NULL;
const char *filename;
@@ -302,7 +290,6 @@ static int img_create(int argc, char **argv)
QEMUOptionParameter *param = NULL, *create_options = NULL;
char *options = NULL;
-flags = 0;
for(;;) {
c = getopt(argc, argv, F:b:f:he6o:);
if (c == -1) {
@@ -323,11 +310,13 @@ static int img_create(int argc, char **argv)
fmt = optarg;
break;
case 'e':
-flags |= BLOCK_FLAG_ENCRYPT;
-break;
+printf(qemu-img: option -e is deprecated, please use \'-o
+ encryption\' instead!\n);
+return -1;
case '6':
-flags |= BLOCK_FLAG_COMPAT6;
-break;
+printf(qemu-img: option -6 is deprecated, please use \'-o
+ compat6\' instead!\n);
+return -1;
case 'o':
options = optarg;
break;
@@ -385,7 +374,7 @@ static int img_create(int argc, char **argv)
}
/* Add old-style options to parameters */
-ret = add_old_style_options(fmt, param, flags, base_filename, base_fmt);
+ret = add_old_style_options(fmt, param, base_filename, base_fmt);
if (ret 0) {
goto out;
}
@@ -674,7 +663,7 @@ static int compare_sectors(const uint8_t *buf1, const
uint8_t *buf2, int n,
static int img_convert(int argc, char **argv)
{
-int c, ret = 0, n, n1, bs_n, bs_i, flags, cluster_size, cluster_sectors;
+int c, ret = 0, n, n1, bs_n, bs_i, compress, cluster_size, cluster_sectors;
const char *fmt, *out_fmt, *out_baseimg, *out_filename;
BlockDriver *drv, *proto_drv;
BlockDriverState **bs = NULL, *out_bs = NULL;
@@ -691,7 +680,7 @@ static int img_convert(int argc, char **argv)
fmt = NULL;
out_fmt = raw;
out_baseimg = NULL;
-flags = 0;
+compress = 0;
for(;;) {
c = getopt(argc, argv, f:O:B:s:hce6o:);
if (c == -1) {
@@ -712,14 +701,16 @@ static int img_convert(int argc, char **argv)
out_baseimg = optarg;
break;
case 'c':
-flags |= BLOCK_FLAG_COMPRESS;
+compress = 1;
break;
case 'e':
-flags |= BLOCK_FLAG_ENCRYPT;
-break;
+printf(qemu-img: option -e is deprecated, please use \'-o
+ encryption\' instead!\n);
+return -1;
case '6':
-flags |= BLOCK_FLAG_COMPAT6;
-break;
+printf(qemu-img: option -6 is deprecated, please use \'-o
+ compat6\' instead!\n);
+return -1;
case 'o':
options = optarg;
break;
@@ -806,7 +797,7 @@ static int img_convert(int argc, char **argv)
}
set_option_parameter_int(param, BLOCK_OPT_SIZE, total_sectors * 512);
-ret = add_old_style_options(out_fmt, param, flags, out_baseimg, NULL);
+ret = add_old_style_options(out_fmt, param, out_baseimg, NULL);
if (ret 0) {
goto out;
}
@@ -818,7
[Qemu-devel] Re: [RFC][PATCH v5 01/21] Move code related to fd handlers into utility functions
On 12/03/10 19:03, Michael Roth wrote: This allows us to implement an i/o loop outside of vl.c that can interact with objects that use qemu_set_fd_handler() Signed-off-by: Michael Roth mdr...@linux.vnet.ibm.com This commit message really tells us nothing. Please be more specific about what is in the commit. diff --git a/qemu-ioh.c b/qemu-ioh.c new file mode 100644 index 000..cc71470 --- /dev/null +++ b/qemu-ioh.c @@ -0,0 +1,115 @@ +/* + * QEMU System Emulator + * + * Copyright (c) 2003-2008 Fabrice Bellard + * + * Permission is hereby granted, free of charge, to any person obtaining a copy + * of this software and associated documentation files (the Software), to deal + * in the Software without restriction, including without limitation the rights + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell + * copies of the Software, and to permit persons to whom the Software is + * furnished to do so, subject to the following conditions: Is this moved or new code? If the former, fine, but if it is new code, you might want to leave your own name on the (c). I presume at least some of the changes are (c) 2010? +/* XXX: fd_read_poll should be suppressed, but an API change is + necessary in the character devices to suppress fd_can_read(). */ XXX in the comment isn't really of much use. Please make it more explicit, or put your name in if it is a statement you wish to make. +int qemu_set_fd_handler3(void *ioh_record_list, + int fd, + IOCanReadHandler *fd_read_poll, + IOHandler *fd_read, + IOHandler *fd_write, + void *opaque) I am not happy with this addition of numbers to these functions, it doesn't tell us why we have a 3 and how it differs from 2. If 3 is really the backend for implementing 2, maybe it would be better to name it __qemu_set_fd_handler2() and then have macros/wrappers calling into it. Cheers, Jes
[Qemu-devel] Re: [RFC][PATCH v5 03/21] virtagent: common code for managing client/server rpc jobs
On 12/03/10 19:03, Michael Roth wrote:
This implements a simple state machine to manage client/server rpc
jobs being multiplexed over a single channel.
A client job consists of sending an rpc request, reading an
rpc response, then making the appropriate callbacks. We allow one
client job to be processed at a time, which will make the following
state transitions:
VA_CLIENT_IDLE - VA_CLIENT_SEND (job queued, send channel open)
VA_CLIENT_SEND - VA_CLIENT_WAIT (request sent, awaiting response)
VA_CLIENT_WAIT - VA_CLIENT_IDLE (response recieved, callbacks made)
A server job consists of recieving an rpc request, generating a
response, then sending the response. We expect to receive one server
request at a time due to the 1 at a time restriction for client jobs.
Server jobs make the following transitions:
VA_SERVER_IDLE - VA_SERVER_WAIT (recieved/executed request, send
channel busy, response deferred)
VA_SERVER_IDLE - VA_SERVER_SEND (recieved/executed request, send
channel open, sending response)
VA_SERVER_WAIT - VA_SERVER_SEND (send channel now open, sending
response)
VA_SERVER_SEND - VA_SERVER_IDLE (response sent)
Signed-off-by: Michael Roth mdr...@linux.vnet.ibm.com
As mentioned before, I really don't understand why this is part of QEMU,
the guest agent really should be able to run totally outside of QEMU.
+
+#define DEBUG_VA
+
+#ifdef DEBUG_VA
+#define TRACE(msg, ...) do { \
+fprintf(stderr, %s:%s():L%d: msg \n, \
+__FILE__, __FUNCTION__, __LINE__, ## __VA_ARGS__); \
+} while(0)
+#else
+#define TRACE(msg, ...) \
+do { } while (0)
+#endif
+
+#define LOG(msg, ...) do { \
+fprintf(stderr, %s:%s(): msg \n, \
+__FILE__, __FUNCTION__, ## __VA_ARGS__); \
+} while(0)
This must be like the 217th copy of these functions, could you please
use some of the code that is already in the tree, and make it generic if
needed.
+
+#define VERSION 1.0
+#define EOL \r\n
+
+#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 1 /* max server rpcs we can queue */
As mentioned last time, please make this stuff configurable and not hard
coded.
Cheers,
Jes
[Qemu-devel] Re: [RFC][PATCH v5 04/21] virtagent: transport definitions and job callbacks
On 12/03/10 19:03, Michael Roth wrote:
+static void va_server_read_cb(const char *content, size_t content_len)
+{
+xmlrpc_mem_block *resp_xml;
+VAServerData *server_data = va_state-server_data;
+int ret;
+
+TRACE(called);
+resp_xml = xmlrpc_registry_process_call(server_data-env,
+server_data-registry,
+NULL, content, content_len);
+if (resp_xml == NULL) {
+LOG(error processing RPC request);
+goto out_bad;
+}
+
+ret = va_server_job_add(resp_xml);
+if (ret != 0) {
+LOG(error adding server job: %s, strerror(ret));
+}
+
+return;
+out_bad:
+/* TODO: should reset state here */
+return;
Looks like some missing error handling is needed here?
+static void va_rpc_parse_hdr(VAHTState *s)
+{
+int i, line_pos = 0;
+bool first_line = true;
+char line_buf[4096];
In 03/21 you defined VA_HDR_LEN_MAX to 4096, here you hard code the
value sounds like something begging to go wrong.
+static int va_end_of_header(char *buf, int end_pos)
+{
+return !strncmp(buf+(end_pos-2), \n\r\n, 3);
+}
Maybe I am missing something here, but it looks like you do a strncmp to
a char that is one past the end of the buffer, or? If this is
intentional, please document it.
All this http parsing code leaves the question open why you do it
manually, instead of relying on a library?
Cheers,
Jes
[Qemu-devel] Re: [RFC][PATCH v5 05/21] virtagent: base client definitions
On 12/03/10 19:03, Michael Roth wrote: +#ifndef VIRTAGENT_H +#define VIRTAGENT_H + +#include monitor.h + +#define GUEST_AGENT_PATH_CLIENT /tmp/virtagent-guest-client.sock +#define HOST_AGENT_PATH_CLIENT /tmp/virtagent-host-client.sock As mentioned last time, this belongs in a config file. Jes
[Qemu-devel] Re: [RFC][PATCH v5 06/21] virtagent: base server definitions
On 12/03/10 19:03, Michael Roth wrote: +#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 Config file please! Jes
[Qemu-devel] Re: [RFC][PATCH v5 07/21] virtagent: add va.getfile RPC
On 12/03/10 19:03, 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.
I think you need to redesign your approach here. see below.
In 06/21 you had:
+#define VA_GETFILE_MAX 1 30
+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);
UH OH!
realloc will do a malloc and a memcpy of the data, this is going to turn
into a really nasty malloc memcpy loop if someone tries to transfer a
large file using this method. You could end up with almost 4GB of
parallel allocations for a guest that might have been configured as a
1GB guest. This would allow the guest to effectively blow the expected
memory consumption out of the water. It's not exactly going to be fast
either :(
Maybe use a tmp file, and write data out to that as you receive it to
avoid the malloc ballooning.
Jes
[Qemu-devel] Re: [RFC][PATCH v5 08/21] virtagent: add agent_viewfile qmp/hmp command
On 12/03/10 19:03, 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.
Signed-off-by: Michael Roth mdr...@linux.vnet.ibm.com
---
hmp-commands.hx | 16 +
monitor.c |1 +
qmp-commands.hx | 33 +++
virtagent.c | 96
+++
virtagent.h |3 ++
5 files changed, 149 insertions(+), 0 deletions(-)
diff --git a/hmp-commands.hx b/hmp-commands.hx
index e5585ba..423c752 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -1212,6 +1212,22 @@ show available trace events and their state
ETEXI
#endif
+{
+.name = agent_viewfile,
+.args_type = filepath:s,
+.params = filepath,
+.help = Echo a file from the guest filesystem,
+.user_print = do_agent_viewfile_print,
+.mhandler.cmd_async = do_agent_viewfile,
+.flags = MONITOR_CMD_ASYNC,
+},
+
+STEXI
+...@item agent_viewfile @var{filepath}
+...@findex agent_viewfile
+Echo the file identified by @var{filepath} on the guest filesystem
+ETEXI
+
STEXI
@end table
ETEXI
diff --git a/monitor.c b/monitor.c
index 8cee35d..145895d 100644
--- a/monitor.c
+++ b/monitor.c
@@ -56,6 +56,7 @@
#include json-parser.h
#include osdep.h
#include exec-all.h
+#include virtagent.h
#ifdef CONFIG_SIMPLE_TRACE
#include trace.h
#endif
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 793cf1c..efa2137 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -738,6 +738,39 @@ Example:
EQMP
{
+.name = agent_viewfile,
+.args_type = filepath:s,
+.params = filepath,
+.help = Echo a file from the guest filesystem,
+.user_print = monitor_user_noop,
+.mhandler.cmd_async = do_agent_viewfile,
+.flags = MONITOR_CMD_ASYNC,
+},
+
+STEXI
+...@item agent_viewfile @var{filepath}
+...@findex agent_viewfile
+Echo the file identified by @var{filepath} on the guest filesystem
+ETEXI
+SQMP
+agent_viewfile
+
+
+Echo the file identified by @var{filepath} from the guest filesystem.
+
+Arguments:
+
+- filepath: Full guest path of the desired file
+
+Example:
+
+- { execute: agent_viewfile,
+arguments: { filepath: /sys/kernel/kexec_loaded } }
+- { return: { contents: 0 } }
+
+EQMP
+
+{
.name = qmp_capabilities,
.args_type = ,
.params = ,
diff --git a/virtagent.c b/virtagent.c
index 34d8545..4a4dc8a 100644
--- a/virtagent.c
+++ b/virtagent.c
@@ -139,3 +139,99 @@ out_free:
out:
return ret;
}
+
+/* QMP/HMP RPC client functions */
+
+void do_agent_viewfile_print(Monitor *mon, const QObject *data)
+{
+QDict *qdict;
+const char *contents = NULL;
+int i;
+
+qdict = qobject_to_qdict(data);
+if (!qdict_haskey(qdict, contents)) {
+return;
+}
+
+contents = qdict_get_str(qdict, contents);
+if (contents != NULL) {
+ /* monitor_printf truncates so do it in chunks. also, file_contents
+ * may not be null-termed at proper location so explicitly calc
+ * last chunk sizes */
+for (i = 0; i strlen(contents); i += 1024) {
+monitor_printf(mon, %.1024s, contents + i);
+}
+}
+monitor_printf(mon, \n);
+}
+
+static void do_agent_viewfile_cb(const char *resp_data,
+ size_t resp_data_len,
+ MonitorCompletion *mon_cb,
+ void *mon_data)
+{
+xmlrpc_value *resp = NULL;
+char *file_contents = NULL;
+size_t file_size;
+int ret;
+xmlrpc_env env;
+QDict *qdict = qdict_new();
+
+if (resp_data == NULL) {
+LOG(error handling RPC request);
+goto out_no_resp;
+}
+
+xmlrpc_env_init(env);
+resp = xmlrpc_parse_response(env, resp_data, resp_data_len);
+if (va_rpc_has_error(env)) {
+ret = -1;
+goto out_no_resp;
+}
+
+xmlrpc_parse_value(env, resp, 6, file_contents, file_size);
+if (va_rpc_has_error(env)) {
+ret = -1;
+goto out;
I believe this suffers from the same architectural problem I mentioned
in my comment to 07/21 - you don't restrict the file size, so it could
blow up the QEMU process on the host trying to view the wrong file.
I really think it is a bad idea to put this kind of command into the
monitor.
Jes
[Qemu-devel] Re: [RFC][PATCH v5 09/21] virtagent: add va.getdmesg RPC
On 12/03/10 19:03, Michael Roth wrote:
Add RPC to view guest dmesg output.
Signed-off-by: Michael Roth mdr...@linux.vnet.ibm.com
---
virtagent-server.c | 46 ++
1 files changed, 46 insertions(+), 0 deletions(-)
diff --git a/virtagent-server.c b/virtagent-server.c
index a430b58..aac8f70 100644
--- a/virtagent-server.c
+++ b/virtagent-server.c
@@ -83,6 +83,50 @@ EXIT_CLOSE_BAD:
return result;
}
+/* va_getdmesg(): return dmesg output
+ * rpc return values:
+ * - dmesg output as a string
+ */
+static xmlrpc_value *va_getdmesg(xmlrpc_env *env,
+ xmlrpc_value *param,
+ void *user_data)
+{
+char *dmesg_buf = NULL, cmd[256];
+int ret;
+xmlrpc_value *result = NULL;
+FILE *pipe;
+
+SLOG(va_getdmesg());
+
+dmesg_buf = qemu_mallocz(VA_DMESG_LEN + 2048);
+sprintf(cmd, dmesg -s %d, VA_DMESG_LEN);
What happens if the guest's dmesg buffer is larger than your hardcoded
value?
Jes
[Qemu-devel] Re: [RFC][PATCH v5 20/21] virtagent: integrate virtagent server/client via chardev
On 12/03/10 19:03, Michael Roth wrote:
+#include virtagent-common.h
+
+static CharDriverState *qemu_chr_open_virtagent(QemuOpts *opts)
+{
+CharDriverState *chr;
+int fd, ret;
+
+/* revert to/enforce default socket chardev options for virtagent */
+if (qemu_opt_get(opts, path) == NULL) {
+qemu_opt_set(opts, path, /tmp/virtagent-client.sock);
+}
More hardcoded paths, which you defined somewhere in a header already.
Again, please make it configurable.
+//qemu_opt_set(opts, id, virtagent);
If it isn't needed, please remove it.
Cheers,
Jes
Re: [Qemu-devel] Re: [RFC][PATCH v5 01/21] Move code related to fd handlers into utility functions
On 12/07/10 15:48, Michael Roth wrote: On 12/07/2010 07:31 AM, Jes Sorensen wrote: On 12/03/10 19:03, Michael Roth wrote: This allows us to implement an i/o loop outside of vl.c that can interact with objects that use qemu_set_fd_handler() Signed-off-by: Michael Rothmdr...@linux.vnet.ibm.com This commit message really tells us nothing. Please be more specific about what is in the commit. Currently, in qemu, the virtagent client/server functionality is driven by vl.c:main_loop_wait(), which implements a select() loop that kicks off handlers registered for various FDs/events via qemu_set_fd_handler(). To share the code with the agent, qemu-va.c, I re-implemented this i/o loop to do same thing, along with vl.c:qemu_set_fd_handler*() and friends. It was big nasty copy/paste job and I think most of the reviewers agreed that the i/o loop code should be shared. This commit moves the shared code into back-end utility functions that get called by vl.c:qemu_set_fd_handler()/qemu_process_fd_handlers() and friends for qemu, and by qemu-tools.c:qemu_set_fd_handler()/qemu_process_fd_handlers() for tools. So now to interact with code that uses qemu_set_fd_handler() you can built a select() loop around these utility functions. Please put some of this in the commit message. I am not happy with this addition of numbers to these functions, it doesn't tell us why we have a 3 and how it differs from 2. If 3 is really the backend for implementing 2, maybe it would be better to name it __qemu_set_fd_handler2() and then have macros/wrappers calling into it. That was the initial plan, but qemu_set_fd_handler2() is a back-end of sorts for qemu_set_fd_handler(), so I was just trying to be consistent with the naming. Personally I don't have any objections one way or the other. Anything to avoid qemu_set_fd_handler17() at some point. I think using __qemu_set_fd_handler() encourages people to modify that code rather than copy it. Cheers, Jes
Re: [Qemu-devel] KVM call agenda for Dec 7
On 12/07/10 00:51, Chris Wright wrote: Please send in any agenda items you are interested in covering. thanks, -chris No agenda, no replies Call canceled I presume? Jes
[Qemu-devel] Re: [PATCH 1/1] qemu-img: Deprecate obsolete -6 and -e options
On 12/07/10 17:02, Kevin Wolf wrote: @@ -323,11 +310,13 @@ static int img_create(int argc, char **argv) fmt = optarg; break; case 'e': -flags |= BLOCK_FLAG_ENCRYPT; -break; +printf(qemu-img: option -e is deprecated, please use \'-o + encryption\' instead!\n); +return -1; The return value of this function is used as exit code of qemu-img, so 1 is probably better than -1. Also, is there a reason why you use printf and not error (which writes the message to stderr)? I looked for fprintf(stderr and found nothing so I used printf() instead. I'm happy to change it to use error() and the return value too. Thanks for the feedback. Cheers, Jes
[Qemu-devel] [PATCH v2 1/1] qemu-img: Deprecate obsolete -6 and -e options
From: Jes Sorensen jes.soren...@redhat.com
If -6 or -e is specified, an error message is printed and we exit. It
does not print help() to avoid the error message getting lost in the
noise.
Signed-off-by: Jes Sorensen jes.soren...@redhat.com
---
block_int.h |1 -
qemu-img.c | 53 ++---
2 files changed, 22 insertions(+), 32 deletions(-)
diff --git a/block_int.h b/block_int.h
index 3c3adb5..3ceed47 100644
--- a/block_int.h
+++ b/block_int.h
@@ -29,7 +29,6 @@
#include qemu-queue.h
#define BLOCK_FLAG_ENCRYPT 1
-#define BLOCK_FLAG_COMPRESS2
#define BLOCK_FLAG_COMPAT6 4
#define BLOCK_OPT_SIZE size
diff --git a/qemu-img.c b/qemu-img.c
index 5b6e648..d146d8c 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -261,21 +261,9 @@ fail:
}
static int add_old_style_options(const char *fmt, QEMUOptionParameter *list,
-int flags, const char *base_filename, const char *base_fmt)
+ const char *base_filename,
+ const char *base_fmt)
{
-if (flags BLOCK_FLAG_ENCRYPT) {
-if (set_option_parameter(list, BLOCK_OPT_ENCRYPT, on)) {
-error(Encryption not supported for file format '%s', fmt);
-return -1;
-}
-}
-if (flags BLOCK_FLAG_COMPAT6) {
-if (set_option_parameter(list, BLOCK_OPT_COMPAT6, on)) {
-error(VMDK version 6 not supported for file format '%s', fmt);
-return -1;
-}
-}
-
if (base_filename) {
if (set_option_parameter(list, BLOCK_OPT_BACKING_FILE, base_filename))
{
error(Backing file not supported for file format '%s', fmt);
@@ -293,7 +281,7 @@ static int add_old_style_options(const char *fmt,
QEMUOptionParameter *list,
static int img_create(int argc, char **argv)
{
-int c, ret = 0, flags;
+int c, ret = 0;
const char *fmt = raw;
const char *base_fmt = NULL;
const char *filename;
@@ -302,7 +290,6 @@ static int img_create(int argc, char **argv)
QEMUOptionParameter *param = NULL, *create_options = NULL;
char *options = NULL;
-flags = 0;
for(;;) {
c = getopt(argc, argv, F:b:f:he6o:);
if (c == -1) {
@@ -323,11 +310,13 @@ static int img_create(int argc, char **argv)
fmt = optarg;
break;
case 'e':
-flags |= BLOCK_FLAG_ENCRYPT;
-break;
+error(qemu-img: option -e is deprecated, please use \'-o
+ encryption\' instead!);
+return 1;
case '6':
-flags |= BLOCK_FLAG_COMPAT6;
-break;
+error(qemu-img: option -6 is deprecated, please use \'-o
+ compat6\' instead!);
+return 1;
case 'o':
options = optarg;
break;
@@ -385,7 +374,7 @@ static int img_create(int argc, char **argv)
}
/* Add old-style options to parameters */
-ret = add_old_style_options(fmt, param, flags, base_filename, base_fmt);
+ret = add_old_style_options(fmt, param, base_filename, base_fmt);
if (ret 0) {
goto out;
}
@@ -674,7 +663,7 @@ static int compare_sectors(const uint8_t *buf1, const
uint8_t *buf2, int n,
static int img_convert(int argc, char **argv)
{
-int c, ret = 0, n, n1, bs_n, bs_i, flags, cluster_size, cluster_sectors;
+int c, ret = 0, n, n1, bs_n, bs_i, compress, cluster_size, cluster_sectors;
const char *fmt, *out_fmt, *out_baseimg, *out_filename;
BlockDriver *drv, *proto_drv;
BlockDriverState **bs = NULL, *out_bs = NULL;
@@ -691,7 +680,7 @@ static int img_convert(int argc, char **argv)
fmt = NULL;
out_fmt = raw;
out_baseimg = NULL;
-flags = 0;
+compress = 0;
for(;;) {
c = getopt(argc, argv, f:O:B:s:hce6o:);
if (c == -1) {
@@ -712,14 +701,16 @@ static int img_convert(int argc, char **argv)
out_baseimg = optarg;
break;
case 'c':
-flags |= BLOCK_FLAG_COMPRESS;
+compress = 1;
break;
case 'e':
-flags |= BLOCK_FLAG_ENCRYPT;
-break;
+error(qemu-img: option -e is deprecated, please use \'-o
+ encryption\' instead!);
+return 1;
case '6':
-flags |= BLOCK_FLAG_COMPAT6;
-break;
+error(qemu-img: option -6 is deprecated, please use \'-o
+ compat6\' instead!);
+return 1;
case 'o':
options = optarg;
break;
@@ -806,7 +797,7 @@ static int img_convert(int argc, char **argv)
}
set_option_parameter_int(param, BLOCK_OPT_SIZE, total_sectors * 512);
-ret = add_old_style_options(out_fmt, param, flags, out_baseimg, NULL);
+ret = add_old_style_options(out_fmt, param, out_baseimg, NULL);
if (ret 0) {
goto out;
}
@@ -818,7 +809,7 @@ static int
[Qemu-devel] [PATCH 1/1] qemu-img.c: Clean up handling of image size in img_create()
From: Jes Sorensen jes.soren...@redhat.com
This cleans up the handling of image size in img_create() by parsing
the value early, and then only setting it once if a value has been
added as the last argument to the command line.
Signed-off-by: Jes Sorensen jes.soren...@redhat.com
---
qemu-img.c | 19 +++
1 files changed, 11 insertions(+), 8 deletions(-)
diff --git a/qemu-img.c b/qemu-img.c
index d146d8c..eaec725 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -282,6 +282,7 @@ static int add_old_style_options(const char *fmt,
QEMUOptionParameter *list,
static int img_create(int argc, char **argv)
{
int c, ret = 0;
+uint64_t img_size = 0;
const char *fmt = raw;
const char *base_fmt = NULL;
const char *filename;
@@ -329,6 +330,11 @@ static int img_create(int argc, char **argv)
}
filename = argv[optind++];
+/* Get image size, if specified */
+if (optind argc) {
+img_size = strtosz(argv[optind++], NULL);
+}
+
if (options !strcmp(options, ?)) {
ret = print_block_option_help(filename, fmt);
goto out;
@@ -356,7 +362,6 @@ static int img_create(int argc, char **argv)
/* Create parameter list with default values */
param = parse_option_parameters(, create_options, param);
-set_option_parameter_int(param, BLOCK_OPT_SIZE, -1);
/* Parse -o options */
if (options) {
@@ -368,21 +373,19 @@ static int img_create(int argc, char **argv)
}
}
-/* Add size to parameters */
-if (optind argc) {
-set_option_parameter(param, BLOCK_OPT_SIZE, argv[optind++]);
-}
-
/* Add old-style options to parameters */
ret = add_old_style_options(fmt, param, base_filename, base_fmt);
if (ret 0) {
goto out;
}
+if (img_size) {
+set_option_parameter_int(param, BLOCK_OPT_SIZE, img_size);
+}
+
// The size for the image must always be specified, with one exception:
// If we are using a backing file, we can obtain the size from there
-if (get_option_parameter(param, BLOCK_OPT_SIZE)-value.n == -1) {
-
+if (get_option_parameter(param, BLOCK_OPT_SIZE)-value.n == 0) {
QEMUOptionParameter *backing_file =
get_option_parameter(param, BLOCK_OPT_BACKING_FILE);
QEMUOptionParameter *backing_fmt =
--
1.7.3.2
Re: [Qemu-devel] [PATCH 1/1] qemu-img.c: Clean up handling of image size in img_create()
On 12/07/10 21:36, Stefan Hajnoczi wrote:
On Tue, Dec 7, 2010 at 5:39 PM, jes.soren...@redhat.com wrote:
// The size for the image must always be specified, with one exception:
// If we are using a backing file, we can obtain the size from there
-if (get_option_parameter(param, BLOCK_OPT_SIZE)-value.n == -1) {
-
+if (get_option_parameter(param, BLOCK_OPT_SIZE)-value.n == 0) {
QEMUOptionParameter *backing_file =
get_option_parameter(param, BLOCK_OPT_BACKING_FILE);
QEMUOptionParameter *backing_fmt =
Today it is possible to create 0 byte sized images. Your patch will
change that:
If there is a backing file, then the size will be taken from the backing file.
If there is no backing file, then an error about missing size will be
printed, even though a size of 0 has been given.
I don't think 0 sized images are very useful, but I'm not sure we
should make this change.
The old code also fails if there is no size, except for when a backing
file is present.
I hadn't thought of the zero sized file, but on the other hand, I don't
see it being useful.
I would like to make this change to get the option handling cleaned up
as it allows me to refactor the code in img_create().
Cheers,
Jes
Re: [Qemu-devel] [PATCH 3/3] Fail if detecting an unknown option
On 12/03/10 12:46, Stefan Hajnoczi wrote: On Thu, Dec 2, 2010 at 5:46 PM, jes.soren...@redhat.com wrote: From: Jes Sorensen jes.soren...@redhat.com This patch changes qemu-img to exit if an unknown option is detected, instead of trying to continue with a set of arguments which may be incorrect. Signed-off-by: Jes Sorensen jes.soren...@redhat.com --- qemu-img.c | 48 1 files changed, 48 insertions(+), 0 deletions(-) Do we get a silent exit if an unknown option is detected? Normally programs print their help/usage when this happens. Stefan Fixed in the next version :) Cheers, Jes
[Qemu-devel] Re: [PATCH 3/3] Fail if detecting an unknown option
On 12/03/10 13:30, Kevin Wolf wrote:
Am 02.12.2010 18:46, schrieb jes.soren...@redhat.com:
diff --git a/qemu-img.c b/qemu-img.c
index d0dc445..f2e1c94 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -304,6 +304,12 @@ static int img_create(int argc, char **argv)
flags = 0;
for(;;) {
c = getopt(argc, argv, F:b:f:he6o:);
+/*
+ * Fail if we detect an unknown argument
+ */
+if (c == '?') {
+return 1;
+}
if (c == -1) {
break;
}
Why not making it another case in the switch statement below instead of
an additional if?
There is a perfectly logical explanation for that. Doing that would
require for me to have clue, which is a bit much to expect :)
That said, we should really do the same for the c == -1 case as well.
Fixed in next version.
Cheers,
Jes
[Qemu-devel] [PATCH v2 0/3] Cleanup qemu-img code
From: Jes Sorensen jes.soren...@redhat.com Hi, These patches moves the handling of block help printing to shared code, which allows the ? detection to happen early in the parsing, instead of half way down img_create() and img_convert(). I would like to see this happen as I would like to pull some of the code out of img_create() and into block.c so it can be shared with qemu and qemu-img. The formatting patch is solely because the third patch wanted to change code next to the badly formatted code, and I didn't want to pollute the patch with the formatting fixed. The third patch fixes qemu-img to exit on detection of unknown options instead of continuing with a potentially wrong set of arguments. New in v2: Add missing free_option_parameters() and handle the help() case in the general switch() statements for the getopt() output. Cheers, Jes Jes Sorensen (3): Consolidate printing of block driver options Fix formatting and missing braces in qemu-img.c Fail if detecting an unknown option qemu-img.c | 132 1 files changed, 97 insertions(+), 35 deletions(-) -- 1.7.3.2
[Qemu-devel] [PATCH 3/3] Fail if detecting an unknown option
From: Jes Sorensen jes.soren...@redhat.com
This patch changes qemu-img to exit if an unknown option is detected,
instead of trying to continue with a set of arguments which may be
incorrect.
Signed-off-by: Jes Sorensen jes.soren...@redhat.com
---
qemu-img.c |8
1 files changed, 8 insertions(+), 0 deletions(-)
diff --git a/qemu-img.c b/qemu-img.c
index 2a54ae2..3e3ca36 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -309,6 +309,7 @@ static int img_create(int argc, char **argv)
break;
}
switch(c) {
+case '?':
case 'h':
help();
break;
@@ -477,6 +478,7 @@ static int img_check(int argc, char **argv)
break;
}
switch(c) {
+case '?':
case 'h':
help();
break;
@@ -555,6 +557,7 @@ static int img_commit(int argc, char **argv)
break;
}
switch(c) {
+case '?':
case 'h':
help();
break;
@@ -693,6 +696,7 @@ static int img_convert(int argc, char **argv)
break;
}
switch(c) {
+case '?':
case 'h':
help();
break;
@@ -1099,6 +1103,7 @@ static int img_info(int argc, char **argv)
break;
}
switch(c) {
+case '?':
case 'h':
help();
break;
@@ -1176,6 +1181,7 @@ static int img_snapshot(int argc, char **argv)
break;
}
switch(c) {
+case '?':
case 'h':
help();
return 0;
@@ -1291,6 +1297,7 @@ static int img_rebase(int argc, char **argv)
break;
}
switch(c) {
+case '?':
case 'h':
help();
return 0;
@@ -1505,6 +1512,7 @@ static int img_resize(int argc, char **argv)
break;
}
switch(c) {
+case '?':
case 'h':
help();
break;
--
1.7.3.2
[Qemu-devel] [PATCH 2/3] Fix formatting and missing braces in qemu-img.c
From: Jes Sorensen jes.soren...@redhat.com
Signed-off-by: Jes Sorensen jes.soren...@redhat.com
Reviewed-by: Stefan Hajnoczi stefa...@linux.vnet.ibm.com
---
qemu-img.c | 77 +++
1 files changed, 51 insertions(+), 26 deletions(-)
diff --git a/qemu-img.c b/qemu-img.c
index 7863835..2a54ae2 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -305,8 +305,9 @@ static int img_create(int argc, char **argv)
flags = 0;
for(;;) {
c = getopt(argc, argv, F:b:f:he6o:);
-if (c == -1)
+if (c == -1) {
break;
+}
switch(c) {
case 'h':
help();
@@ -333,8 +334,9 @@ static int img_create(int argc, char **argv)
}
/* Get the filename */
-if (optind = argc)
+if (optind = argc) {
help();
+}
filename = argv[optind++];
if (options !strcmp(options, ?)) {
@@ -471,8 +473,9 @@ static int img_check(int argc, char **argv)
fmt = NULL;
for(;;) {
c = getopt(argc, argv, f:h);
-if (c == -1)
+if (c == -1) {
break;
+}
switch(c) {
case 'h':
help();
@@ -482,8 +485,9 @@ static int img_check(int argc, char **argv)
break;
}
}
-if (optind = argc)
+if (optind = argc) {
help();
+}
filename = argv[optind++];
bs = bdrv_new_open(filename, fmt, BDRV_O_FLAGS);
@@ -547,8 +551,9 @@ static int img_commit(int argc, char **argv)
fmt = NULL;
for(;;) {
c = getopt(argc, argv, f:h);
-if (c == -1)
+if (c == -1) {
break;
+}
switch(c) {
case 'h':
help();
@@ -558,8 +563,9 @@ static int img_commit(int argc, char **argv)
break;
}
}
-if (optind = argc)
+if (optind = argc) {
help();
+}
filename = argv[optind++];
bs = bdrv_new_open(filename, fmt, BDRV_O_FLAGS | BDRV_O_RDWR);
@@ -683,8 +689,9 @@ static int img_convert(int argc, char **argv)
flags = 0;
for(;;) {
c = getopt(argc, argv, f:O:B:s:hce6o:);
-if (c == -1)
+if (c == -1) {
break;
+}
switch(c) {
case 'h':
help();
@@ -717,7 +724,9 @@ static int img_convert(int argc, char **argv)
}
bs_n = argc - optind - 1;
-if (bs_n 1) help();
+if (bs_n 1) {
+help();
+}
out_filename = argv[argc - 1];
@@ -908,8 +917,9 @@ static int img_convert(int argc, char **argv)
}
assert (remainder == 0);
-if (n cluster_sectors)
+if (n cluster_sectors) {
memset(buf + n * 512, 0, cluster_size - n * 512);
+}
if (is_not_zero(buf, cluster_size)) {
ret = bdrv_write_compressed(out_bs, sector_num, buf,
cluster_sectors);
@@ -929,12 +939,14 @@ static int img_convert(int argc, char **argv)
sector_num = 0; // total number of sectors converted so far
for(;;) {
nb_sectors = total_sectors - sector_num;
-if (nb_sectors = 0)
+if (nb_sectors = 0) {
break;
-if (nb_sectors = (IO_BUF_SIZE / 512))
+}
+if (nb_sectors = (IO_BUF_SIZE / 512)) {
n = (IO_BUF_SIZE / 512);
-else
+} else {
n = nb_sectors;
+}
while (sector_num - bs_offset = bs_sectors) {
bs_i ++;
@@ -946,8 +958,9 @@ static int img_convert(int argc, char **argv)
sector_num, bs_i, bs_offset, bs_sectors); */
}
-if (n bs_offset + bs_sectors - sector_num)
+if (n bs_offset + bs_sectors - sector_num) {
n = bs_offset + bs_sectors - sector_num;
+}
if (has_zero_init) {
/* If the output image is being created as a copy on write
image,
@@ -1082,8 +1095,9 @@ static int img_info(int argc, char **argv)
fmt = NULL;
for(;;) {
c = getopt(argc, argv, f:h);
-if (c == -1)
+if (c == -1) {
break;
+}
switch(c) {
case 'h':
help();
@@ -1093,8 +1107,9 @@ static int img_info(int argc, char **argv)
break;
}
}
-if (optind = argc)
+if (optind = argc) {
help();
+}
filename = argv[optind++];
bs = bdrv_new_open(filename, fmt, BDRV_O_FLAGS | BDRV_O_NO_BACKING);
@@ -1105,11 +1120,12 @@ static int img_info(int argc, char **argv)
bdrv_get_geometry(bs, total_sectors);
get_human_readable_size(size_buf, sizeof(size_buf), total_sectors * 512);
allocated_size = get_allocated_file_size(filename);
-if (allocated_size 0)
+if (allocated_size 0) {
snprintf(dsize_buf, sizeof(dsize_buf), unavailable
[Qemu-devel] [PATCH 1/3] Consolidate printing of block driver options
From: Jes Sorensen jes.soren...@redhat.com
This consolidates the printing of block driver options in
print_block_option_help() which is called from both img_create() and
img_convert().
This allows for the ? detection to be done just after the parsing of
options and the filename, instead of half way down the codepath of
these functions.
Signed-off-by: Jes Sorensen jes.soren...@redhat.com
---
qemu-img.c | 47 ++-
1 files changed, 38 insertions(+), 9 deletions(-)
diff --git a/qemu-img.c b/qemu-img.c
index fa77ac0..7863835 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -188,6 +188,33 @@ static int read_password(char *buf, int buf_size)
}
#endif
+static int print_block_option_help(const char *filename, const char *fmt)
+{
+BlockDriver *drv, *proto_drv;
+QEMUOptionParameter *create_options = NULL;
+
+/* Find driver and parse its options */
+drv = bdrv_find_format(fmt);
+if (!drv) {
+error(Unknown file format '%s', fmt);
+return 1;
+}
+
+proto_drv = bdrv_find_protocol(filename);
+if (!proto_drv) {
+error(Unknown protocol '%s', filename);
+return 1;
+}
+
+create_options = append_option_parameters(create_options,
+ drv-create_options);
+create_options = append_option_parameters(create_options,
+ proto_drv-create_options);
+print_option_help(create_options);
+free_option_parameters(create_options);
+return 0;
+}
+
static BlockDriverState *bdrv_new_open(const char *filename,
const char *fmt,
int flags)
@@ -310,6 +337,11 @@ static int img_create(int argc, char **argv)
help();
filename = argv[optind++];
+if (options !strcmp(options, ?)) {
+ret = print_block_option_help(filename, fmt);
+goto out;
+}
+
/* Find driver and parse its options */
drv = bdrv_find_format(fmt);
if (!drv) {
@@ -328,11 +360,6 @@ static int img_create(int argc, char **argv)
create_options = append_option_parameters(create_options,
proto_drv-create_options);
-if (options !strcmp(options, ?)) {
-print_option_help(create_options);
-goto out;
-}
-
/* Create parameter list with default values */
param = parse_option_parameters(, create_options, param);
set_option_parameter_int(param, BLOCK_OPT_SIZE, -1);
@@ -694,6 +721,11 @@ static int img_convert(int argc, char **argv)
out_filename = argv[argc - 1];
+if (options !strcmp(options, ?)) {
+ret = print_block_option_help(out_filename, out_fmt);
+goto out2;
+}
+
if (bs_n 1 out_baseimg) {
error(-B makes no sense when concatenating multiple input images);
return 1;
@@ -749,10 +781,6 @@ static int img_convert(int argc, char **argv)
drv-create_options);
create_options = append_option_parameters(create_options,
proto_drv-create_options);
-if (options !strcmp(options, ?)) {
-print_option_help(create_options);
-goto out;
-}
if (options) {
param = parse_option_parameters(options, create_options, param);
@@ -984,6 +1012,7 @@ out:
}
}
free(bs);
+out2:
if (ret) {
return 1;
}
--
1.7.3.2
[Qemu-devel] Re: [PATCH 3/3] Fail if detecting an unknown option
On 12/06/10 10:37, Kevin Wolf wrote: Am 06.12.2010 09:02, schrieb Jes Sorensen: On 12/03/10 13:30, Kevin Wolf wrote: There is a perfectly logical explanation for that. Doing that would require for me to have clue, which is a bit much to expect :) That said, we should really do the same for the c == -1 case as well. That's what I thought at first, too. But then the break relates to the switch instead of the for, so it would have to become a goto to a new label. Probably not a big improvement... Yeah, it hit me the moment I hit send, so ignore that comment. Cheers, Jes
Re: [Qemu-devel] [PATCH 1/3] Consolidate printing of block driver options
On 12/06/10 10:32, Stefan Hajnoczi wrote:
On Mon, Dec 6, 2010 at 8:17 AM, jes.soren...@redhat.com wrote:
@@ -694,6 +721,11 @@ static int img_convert(int argc, char **argv)
out_filename = argv[argc - 1];
+if (options !strcmp(options, ?)) {
+ret = print_block_option_help(out_filename, out_fmt);
+goto out2;
+}
+
if (bs_n 1 out_baseimg) {
error(-B makes no sense when concatenating multiple input images);
return 1;
Why goto out2 and not just return like the bs 1 out_baseimg check?
It is cleaner, I'd rather convert the bs_n test to do it too.
Cheers,
Jes
Re: [Qemu-devel] [PATCH 1/3] Consolidate printing of block driver options
On 12/06/10 11:37, Stefan Hajnoczi wrote: On Mon, Dec 6, 2010 at 10:20 AM, Jes Sorensen jes.soren...@redhat.com wrote: On 12/06/10 10:32, Stefan Hajnoczi wrote: On Mon, Dec 6, 2010 at 8:17 AM, jes.soren...@redhat.com wrote: Why goto out2 and not just return like the bs 1 out_baseimg check? It is cleaner, I'd rather convert the bs_n test to do it too. out2 tells me nothing and is just indirection for a return. At this point no resources have been acquired and it is simplest to bail out early. A consistent out path is more likely to catch issues if the code is modified later. I am not a big fan of the random mix of return vs goto out that we spray over the code or having help() call exit() for that matter. Cheers, Jes
[Qemu-devel] [PATCH v3 0/7] Cleanup qemu-img code
From: Jes Sorensen jes.soren...@redhat.com Hi, These patches applies a number of cleanups to qemu-img.c as well as a minor bug in qemu-malloc.c. The handling of block help printing is moved to shared code, which allows the ? detection to happen early in the parsing, instead of half way down img_create() and img_convert(). I would like to see this happen as I would like to pull some of the code out of img_create() and into block.c so it can be shared with qemu and qemu-img. In addition there is a couple of patches to clean up the error handling in qemu-img.c and make it more consistent. The formatting patch is solely because the last patch wanted to change code next to the badly formatted code, and I didn't want to pollute the patch with the formatting fixed. The seventh patch fixes qemu-img to exit on detection of unknown options instead of continuing with a potentially wrong set of arguments. v3 applies a number of changes discussed on irc and email. This is the grow to seven from three patches series. Cheers, Jes Jes Sorensen (7): Add missing tracing to qemu_mallocz() Use qemu_mallocz() instead of calloc() in img_convert() img_convert(): Only try to free bs[] entries if bs is valid. Make error handling more consistent in img_create() and img_resize() Consolidate printing of block driver options Fix formatting and missing braces in qemu-img.c Fail if detecting an unknown option qemu-img.c| 162 +++- qemu-malloc.c |5 ++- 2 files changed, 117 insertions(+), 50 deletions(-) -- 1.7.3.2
[Qemu-devel] [PATCH 1/7] Add missing tracing to qemu_mallocz()
From: Jes Sorensen jes.soren...@redhat.com
Signed-off-by: Jes Sorensen jes.soren...@redhat.com
---
qemu-malloc.c |5 -
1 files changed, 4 insertions(+), 1 deletions(-)
diff --git a/qemu-malloc.c b/qemu-malloc.c
index 28fb05a..b9b3851 100644
--- a/qemu-malloc.c
+++ b/qemu-malloc.c
@@ -64,10 +64,13 @@ void *qemu_realloc(void *ptr, size_t size)
void *qemu_mallocz(size_t size)
{
+void *ptr;
if (!size !allow_zero_malloc()) {
abort();
}
-return qemu_oom_check(calloc(1, size ? size : 1));
+ptr = qemu_oom_check(calloc(1, size ? size : 1));
+trace_qemu_malloc(size, ptr);
+return ptr;
}
char *qemu_strdup(const char *str)
--
1.7.3.2
[Qemu-devel] [PATCH 2/7] Use qemu_mallocz() instead of calloc() in img_convert()
From: Jes Sorensen jes.soren...@redhat.com
Signed-off-by: Jes Sorensen jes.soren...@redhat.com
---
qemu-img.c |8 ++--
1 files changed, 2 insertions(+), 6 deletions(-)
diff --git a/qemu-img.c b/qemu-img.c
index fa77ac0..eca99c4 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -699,11 +699,7 @@ static int img_convert(int argc, char **argv)
return 1;
}
-bs = calloc(bs_n, sizeof(BlockDriverState *));
-if (!bs) {
-error(Out of memory);
-return 1;
-}
+bs = qemu_mallocz(bs_n * sizeof(BlockDriverState *));
total_sectors = 0;
for (bs_i = 0; bs_i bs_n; bs_i++) {
@@ -983,7 +979,7 @@ out:
bdrv_delete(bs[bs_i]);
}
}
-free(bs);
+qemu_free(bs);
if (ret) {
return 1;
}
--
1.7.3.2
[Qemu-devel] [PATCH 5/7] Consolidate printing of block driver options
From: Jes Sorensen jes.soren...@redhat.com
This consolidates the printing of block driver options in
print_block_option_help() which is called from both img_create() and
img_convert().
This allows for the ? detection to be done just after the parsing of
options and the filename, instead of half way down the codepath of
these functions.
Signed-off-by: Jes Sorensen jes.soren...@redhat.com
---
qemu-img.c | 46 +-
1 files changed, 37 insertions(+), 9 deletions(-)
diff --git a/qemu-img.c b/qemu-img.c
index 7f4939e..c7d0ca8 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -188,6 +188,33 @@ static int read_password(char *buf, int buf_size)
}
#endif
+static int print_block_option_help(const char *filename, const char *fmt)
+{
+BlockDriver *drv, *proto_drv;
+QEMUOptionParameter *create_options = NULL;
+
+/* Find driver and parse its options */
+drv = bdrv_find_format(fmt);
+if (!drv) {
+error(Unknown file format '%s', fmt);
+return 1;
+}
+
+proto_drv = bdrv_find_protocol(filename);
+if (!proto_drv) {
+error(Unknown protocol '%s', filename);
+return 1;
+}
+
+create_options = append_option_parameters(create_options,
+ drv-create_options);
+create_options = append_option_parameters(create_options,
+ proto_drv-create_options);
+print_option_help(create_options);
+free_option_parameters(create_options);
+return 0;
+}
+
static BlockDriverState *bdrv_new_open(const char *filename,
const char *fmt,
int flags)
@@ -310,6 +337,11 @@ static int img_create(int argc, char **argv)
help();
filename = argv[optind++];
+if (options !strcmp(options, ?)) {
+ret = print_block_option_help(filename, fmt);
+goto out;
+}
+
/* Find driver and parse its options */
drv = bdrv_find_format(fmt);
if (!drv) {
@@ -330,11 +362,6 @@ static int img_create(int argc, char **argv)
create_options = append_option_parameters(create_options,
proto_drv-create_options);
-if (options !strcmp(options, ?)) {
-print_option_help(create_options);
-goto out;
-}
-
/* Create parameter list with default values */
param = parse_option_parameters(, create_options, param);
set_option_parameter_int(param, BLOCK_OPT_SIZE, -1);
@@ -696,6 +723,11 @@ static int img_convert(int argc, char **argv)
out_filename = argv[argc - 1];
+if (options !strcmp(options, ?)) {
+ret = print_block_option_help(out_filename, out_fmt);
+goto out;
+}
+
if (bs_n 1 out_baseimg) {
error(-B makes no sense when concatenating multiple input images);
ret = -1;
@@ -748,10 +780,6 @@ static int img_convert(int argc, char **argv)
drv-create_options);
create_options = append_option_parameters(create_options,
proto_drv-create_options);
-if (options !strcmp(options, ?)) {
-print_option_help(create_options);
-goto out;
-}
if (options) {
param = parse_option_parameters(options, create_options, param);
--
1.7.3.2
[Qemu-devel] [PATCH 4/7] Make error handling more consistent in img_create() and img_resize()
From: Jes Sorensen jes.soren...@redhat.com
Signed-off-by: Jes Sorensen jes.soren...@redhat.com
---
qemu-img.c | 12
1 files changed, 8 insertions(+), 4 deletions(-)
diff --git a/qemu-img.c b/qemu-img.c
index aded72d..7f4939e 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -314,13 +314,15 @@ static int img_create(int argc, char **argv)
drv = bdrv_find_format(fmt);
if (!drv) {
error(Unknown file format '%s', fmt);
-return 1;
+ret = -1;
+goto out;
}
proto_drv = bdrv_find_protocol(filename);
if (!proto_drv) {
error(Unknown protocol '%s', filename);
-return 1;
+ret = -1;
+goto out;
}
create_options = append_option_parameters(create_options,
@@ -1483,14 +1485,16 @@ static int img_resize(int argc, char **argv)
param = parse_option_parameters(, resize_options, NULL);
if (set_option_parameter(param, BLOCK_OPT_SIZE, size)) {
/* Error message already printed when size parsing fails */
-exit(1);
+ret = -1;
+goto out;
}
n = get_option_parameter(param, BLOCK_OPT_SIZE)-value.n;
free_option_parameters(param);
bs = bdrv_new_open(filename, fmt, BDRV_O_FLAGS | BDRV_O_RDWR);
if (!bs) {
-return 1;
+ret = -1;
+goto out;
}
if (relative) {
--
1.7.3.2
[Qemu-devel] [PATCH 6/7] Fix formatting and missing braces in qemu-img.c
From: Jes Sorensen jes.soren...@redhat.com
Signed-off-by: Jes Sorensen jes.soren...@redhat.com
Reviewed-by: Stefan Hajnoczi stefa...@linux.vnet.ibm.com
---
qemu-img.c | 77 +++
1 files changed, 51 insertions(+), 26 deletions(-)
diff --git a/qemu-img.c b/qemu-img.c
index c7d0ca8..d812db0 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -305,8 +305,9 @@ static int img_create(int argc, char **argv)
flags = 0;
for(;;) {
c = getopt(argc, argv, F:b:f:he6o:);
-if (c == -1)
+if (c == -1) {
break;
+}
switch(c) {
case 'h':
help();
@@ -333,8 +334,9 @@ static int img_create(int argc, char **argv)
}
/* Get the filename */
-if (optind = argc)
+if (optind = argc) {
help();
+}
filename = argv[optind++];
if (options !strcmp(options, ?)) {
@@ -473,8 +475,9 @@ static int img_check(int argc, char **argv)
fmt = NULL;
for(;;) {
c = getopt(argc, argv, f:h);
-if (c == -1)
+if (c == -1) {
break;
+}
switch(c) {
case 'h':
help();
@@ -484,8 +487,9 @@ static int img_check(int argc, char **argv)
break;
}
}
-if (optind = argc)
+if (optind = argc) {
help();
+}
filename = argv[optind++];
bs = bdrv_new_open(filename, fmt, BDRV_O_FLAGS);
@@ -549,8 +553,9 @@ static int img_commit(int argc, char **argv)
fmt = NULL;
for(;;) {
c = getopt(argc, argv, f:h);
-if (c == -1)
+if (c == -1) {
break;
+}
switch(c) {
case 'h':
help();
@@ -560,8 +565,9 @@ static int img_commit(int argc, char **argv)
break;
}
}
-if (optind = argc)
+if (optind = argc) {
help();
+}
filename = argv[optind++];
bs = bdrv_new_open(filename, fmt, BDRV_O_FLAGS | BDRV_O_RDWR);
@@ -685,8 +691,9 @@ static int img_convert(int argc, char **argv)
flags = 0;
for(;;) {
c = getopt(argc, argv, f:O:B:s:hce6o:);
-if (c == -1)
+if (c == -1) {
break;
+}
switch(c) {
case 'h':
help();
@@ -719,7 +726,9 @@ static int img_convert(int argc, char **argv)
}
bs_n = argc - optind - 1;
-if (bs_n 1) help();
+if (bs_n 1) {
+help();
+}
out_filename = argv[argc - 1];
@@ -907,8 +916,9 @@ static int img_convert(int argc, char **argv)
}
assert (remainder == 0);
-if (n cluster_sectors)
+if (n cluster_sectors) {
memset(buf + n * 512, 0, cluster_size - n * 512);
+}
if (is_not_zero(buf, cluster_size)) {
ret = bdrv_write_compressed(out_bs, sector_num, buf,
cluster_sectors);
@@ -928,12 +938,14 @@ static int img_convert(int argc, char **argv)
sector_num = 0; // total number of sectors converted so far
for(;;) {
nb_sectors = total_sectors - sector_num;
-if (nb_sectors = 0)
+if (nb_sectors = 0) {
break;
-if (nb_sectors = (IO_BUF_SIZE / 512))
+}
+if (nb_sectors = (IO_BUF_SIZE / 512)) {
n = (IO_BUF_SIZE / 512);
-else
+} else {
n = nb_sectors;
+}
while (sector_num - bs_offset = bs_sectors) {
bs_i ++;
@@ -945,8 +957,9 @@ static int img_convert(int argc, char **argv)
sector_num, bs_i, bs_offset, bs_sectors); */
}
-if (n bs_offset + bs_sectors - sector_num)
+if (n bs_offset + bs_sectors - sector_num) {
n = bs_offset + bs_sectors - sector_num;
+}
if (has_zero_init) {
/* If the output image is being created as a copy on write
image,
@@ -1082,8 +1095,9 @@ static int img_info(int argc, char **argv)
fmt = NULL;
for(;;) {
c = getopt(argc, argv, f:h);
-if (c == -1)
+if (c == -1) {
break;
+}
switch(c) {
case 'h':
help();
@@ -1093,8 +1107,9 @@ static int img_info(int argc, char **argv)
break;
}
}
-if (optind = argc)
+if (optind = argc) {
help();
+}
filename = argv[optind++];
bs = bdrv_new_open(filename, fmt, BDRV_O_FLAGS | BDRV_O_NO_BACKING);
@@ -1105,11 +1120,12 @@ static int img_info(int argc, char **argv)
bdrv_get_geometry(bs, total_sectors);
get_human_readable_size(size_buf, sizeof(size_buf), total_sectors * 512);
allocated_size = get_allocated_file_size(filename);
-if (allocated_size 0)
+if (allocated_size 0) {
snprintf(dsize_buf, sizeof(dsize_buf), unavailable
[Qemu-devel] [PATCH 3/7] img_convert(): Only try to free bs[] entries if bs is valid.
From: Jes Sorensen jes.soren...@redhat.com
This allows for jumping to 'out:' consistently for error exit.
Signed-off-by: Jes Sorensen jes.soren...@redhat.com
---
qemu-img.c | 13 -
1 files changed, 8 insertions(+), 5 deletions(-)
diff --git a/qemu-img.c b/qemu-img.c
index eca99c4..aded72d 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -696,7 +696,8 @@ static int img_convert(int argc, char **argv)
if (bs_n 1 out_baseimg) {
error(-B makes no sense when concatenating multiple input images);
-return 1;
+ret = -1;
+goto out;
}
bs = qemu_mallocz(bs_n * sizeof(BlockDriverState *));
@@ -974,12 +975,14 @@ out:
if (out_bs) {
bdrv_delete(out_bs);
}
-for (bs_i = 0; bs_i bs_n; bs_i++) {
-if (bs[bs_i]) {
-bdrv_delete(bs[bs_i]);
+if (bs) {
+for (bs_i = 0; bs_i bs_n; bs_i++) {
+if (bs[bs_i]) {
+bdrv_delete(bs[bs_i]);
+}
}
+qemu_free(bs);
}
-qemu_free(bs);
if (ret) {
return 1;
}
--
1.7.3.2
[Qemu-devel] [PATCH 7/7] Fail if detecting an unknown option
From: Jes Sorensen jes.soren...@redhat.com
This patch changes qemu-img to exit if an unknown option is detected,
instead of trying to continue with a set of arguments which may be
incorrect.
Signed-off-by: Jes Sorensen jes.soren...@redhat.com
---
qemu-img.c |8
1 files changed, 8 insertions(+), 0 deletions(-)
diff --git a/qemu-img.c b/qemu-img.c
index d812db0..f021a06 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -309,6 +309,7 @@ static int img_create(int argc, char **argv)
break;
}
switch(c) {
+case '?':
case 'h':
help();
break;
@@ -479,6 +480,7 @@ static int img_check(int argc, char **argv)
break;
}
switch(c) {
+case '?':
case 'h':
help();
break;
@@ -557,6 +559,7 @@ static int img_commit(int argc, char **argv)
break;
}
switch(c) {
+case '?':
case 'h':
help();
break;
@@ -695,6 +698,7 @@ static int img_convert(int argc, char **argv)
break;
}
switch(c) {
+case '?':
case 'h':
help();
break;
@@ -1099,6 +1103,7 @@ static int img_info(int argc, char **argv)
break;
}
switch(c) {
+case '?':
case 'h':
help();
break;
@@ -1176,6 +1181,7 @@ static int img_snapshot(int argc, char **argv)
break;
}
switch(c) {
+case '?':
case 'h':
help();
return 0;
@@ -1291,6 +1297,7 @@ static int img_rebase(int argc, char **argv)
break;
}
switch(c) {
+case '?':
case 'h':
help();
return 0;
@@ -1505,6 +1512,7 @@ static int img_resize(int argc, char **argv)
break;
}
switch(c) {
+case '?':
case 'h':
help();
break;
--
1.7.3.2
[Qemu-devel] Re: [PATCH 4/7] Make error handling more consistent in img_create() and img_resize()
On 12/06/10 16:25, Kevin Wolf wrote:
}
n = get_option_parameter(param, BLOCK_OPT_SIZE)-value.n;
free_option_parameters(param);
bs = bdrv_new_open(filename, fmt, BDRV_O_FLAGS | BDRV_O_RDWR);
if (!bs) {
-return 1;
+ret = -1;
+goto out;
}
Same here.
Heh, wanted to try it out to be sure, but the compiler notices that, so
it doesn't even build:
Gr I am an idiot!
Sorry about the noise, I was sure I had tested that last change. Fix
coming up in a few minutes.
Cheers,
Jes
[Qemu-devel] [PATCH 4/7] Make error handling more consistent in img_create() and img_resize()
From: Jes Sorensen jes.soren...@redhat.com
Signed-off-by: Jes Sorensen jes.soren...@redhat.com
---
qemu-img.c | 14 +-
1 files changed, 9 insertions(+), 5 deletions(-)
diff --git a/qemu-img.c b/qemu-img.c
index aded72d..2deac67 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -314,13 +314,15 @@ static int img_create(int argc, char **argv)
drv = bdrv_find_format(fmt);
if (!drv) {
error(Unknown file format '%s', fmt);
-return 1;
+ret = -1;
+goto out;
}
proto_drv = bdrv_find_protocol(filename);
if (!proto_drv) {
error(Unknown protocol '%s', filename);
-return 1;
+ret = -1;
+goto out;
}
create_options = append_option_parameters(create_options,
@@ -1432,7 +1434,7 @@ static int img_resize(int argc, char **argv)
int c, ret, relative;
const char *filename, *fmt, *size;
int64_t n, total_size;
-BlockDriverState *bs;
+BlockDriverState *bs = NULL;
QEMUOptionParameter *param;
QEMUOptionParameter resize_options[] = {
{
@@ -1483,14 +1485,16 @@ static int img_resize(int argc, char **argv)
param = parse_option_parameters(, resize_options, NULL);
if (set_option_parameter(param, BLOCK_OPT_SIZE, size)) {
/* Error message already printed when size parsing fails */
-exit(1);
+ret = -1;
+goto out;
}
n = get_option_parameter(param, BLOCK_OPT_SIZE)-value.n;
free_option_parameters(param);
bs = bdrv_new_open(filename, fmt, BDRV_O_FLAGS | BDRV_O_RDWR);
if (!bs) {
-return 1;
+ret = -1;
+goto out;
}
if (relative) {
--
1.7.3.2
[Qemu-devel] Re: [PATCH 4/7] Make error handling more consistent in img_create() and img_resize()
On 12/06/10 16:57, Kevin Wolf wrote: bdrv_delete doesn't check for NULL, so this still isn't enough. Try something like qemu-img resize -f vmdx foo +0 and you'll get a segfault. G :( It's a bummer things are so inconsistent throughout QEMU, most of the free() functions can handle it. Updated patch in a minute - sorry. Thanks, Jes
[Qemu-devel] [PATCH v5 4/7] Make error handling more consistent in img_create() and img_resize()
From: Jes Sorensen jes.soren...@redhat.com
Signed-off-by: Jes Sorensen jes.soren...@redhat.com
---
qemu-img.c | 18 --
1 files changed, 12 insertions(+), 6 deletions(-)
diff --git a/qemu-img.c b/qemu-img.c
index aded72d..6b2b18b 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -314,13 +314,15 @@ static int img_create(int argc, char **argv)
drv = bdrv_find_format(fmt);
if (!drv) {
error(Unknown file format '%s', fmt);
-return 1;
+ret = -1;
+goto out;
}
proto_drv = bdrv_find_protocol(filename);
if (!proto_drv) {
error(Unknown protocol '%s', filename);
-return 1;
+ret = -1;
+goto out;
}
create_options = append_option_parameters(create_options,
@@ -1432,7 +1434,7 @@ static int img_resize(int argc, char **argv)
int c, ret, relative;
const char *filename, *fmt, *size;
int64_t n, total_size;
-BlockDriverState *bs;
+BlockDriverState *bs = NULL;
QEMUOptionParameter *param;
QEMUOptionParameter resize_options[] = {
{
@@ -1483,14 +1485,16 @@ static int img_resize(int argc, char **argv)
param = parse_option_parameters(, resize_options, NULL);
if (set_option_parameter(param, BLOCK_OPT_SIZE, size)) {
/* Error message already printed when size parsing fails */
-exit(1);
+ret = -1;
+goto out;
}
n = get_option_parameter(param, BLOCK_OPT_SIZE)-value.n;
free_option_parameters(param);
bs = bdrv_new_open(filename, fmt, BDRV_O_FLAGS | BDRV_O_RDWR);
if (!bs) {
-return 1;
+ret = -1;
+goto out;
}
if (relative) {
@@ -1520,7 +1524,9 @@ static int img_resize(int argc, char **argv)
break;
}
out:
-bdrv_delete(bs);
+if (bs) {
+bdrv_delete(bs);
+}
if (ret) {
return 1;
}
--
1.7.3.2
