* [PATCH] xl: avoid creating domains with duplicate names
@ 2011-01-25 15:15 Stefano Stabellini
2011-01-25 18:21 ` Ian Jackson
0 siblings, 1 reply; 7+ messages in thread
From: Stefano Stabellini @ 2011-01-25 15:15 UTC (permalink / raw)
To: xen-devel
Do not create the domain if another domain with the same name is already
running.
Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
diff -r 829855751e19 tools/libxl/xl_cmdimpl.c
--- a/tools/libxl/xl_cmdimpl.c Tue Jan 25 15:05:20 2011 +0000
+++ b/tools/libxl/xl_cmdimpl.c Tue Jan 25 15:12:21 2011 +0000
@@ -569,6 +569,7 @@ static void parse_config_data(const char
XLU_ConfigList *vbds, *nics, *pcis, *cvfbs, *net2s, *cpuids;
int pci_power_mgmt = 0;
int pci_msitranslate = 1;
+ uint32_t domid_e;
int e;
libxl_domain_create_info *c_info = &d_config->c_info;
@@ -598,6 +599,11 @@ static void parse_config_data(const char
if (xlu_cfg_replace_string (config, "name", &c_info->name))
c_info->name = strdup("test");
+ e = libxl_name_to_domid(&ctx, c_info->name, &domid_e);
+ if (!e) {
+ fprintf(stderr, "A domain with name \"%s\" already exists.\n", c_info->name);
+ exit(1);
+ }
if (!xlu_cfg_get_string (config, "uuid", &buf) ) {
if ( libxl_uuid_from_string(&c_info->uuid, buf) ) {
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] xl: avoid creating domains with duplicate names
2011-01-25 15:15 [PATCH] xl: avoid creating domains with duplicate names Stefano Stabellini
@ 2011-01-25 18:21 ` Ian Jackson
2011-01-26 10:31 ` Stefano Stabellini
0 siblings, 1 reply; 7+ messages in thread
From: Ian Jackson @ 2011-01-25 18:21 UTC (permalink / raw)
To: Stefano Stabellini; +Cc: xen-devel
Stefano Stabellini writes ("[Xen-devel] [PATCH] xl: avoid creating domains with duplicate names"):
> Do not create the domain if another domain with the same name is already
> running.
Thanks. I approve of the principle of this patch, but:
> + e = libxl_name_to_domid(&ctx, c_info->name, &domid_e);
> + if (!e) {
You should explicitly check the actual error return value of
libxl_name_to_domid and check that it is the expected error code, and
not some other error code meaning "general failure" or something.
I went to look at the code for libxl_name_to_domid and it returns,
entirely ad-hoc, -1 (which is now ERROR_VERSION), for "no such
domain".
IMO it should return ERROR_INVAL.
I grepped the libxl source for "-1" and found that this practice is
widespread. At this stage of the release I don't want to risk
breaking everything by changing them all (since something may compare
with -1, or something).
So I suggest the attached fixup patch, and then a revised version of
your patch. What do you think?
Ian.
libxl: band-aid for functions with return literal "-1"
Many libxl functions erroneously return "-1" on error, rather than
some ERROR_* value.
To deal with this, invent a new ERROR_NONSPECIFIC "-1" which indicates
that "the function which generated this error code is broken".
Fix up the one we care about for forthcoming duplicate domain
detection (libxl_name_to_domid) and the others following the same
pattern nearby; leave the rest for post-4.1.
Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
diff -r 1d1eec7e1fb4 tools/libxl/libxl.h
--- a/tools/libxl/libxl.h Tue Jan 25 18:09:49 2011 +0000
+++ b/tools/libxl/libxl.h Tue Jan 25 18:18:53 2011 +0000
@@ -232,12 +232,13 @@ typedef struct {
} libxl_domain_suspend_info;
enum {
- ERROR_VERSION = -1,
- ERROR_FAIL = -2,
- ERROR_NI = -3,
- ERROR_NOMEM = -4,
- ERROR_INVAL = -5,
- ERROR_BADFAIL = -6,
+ ERROR_NONSPECIFIC = -1,
+ ERROR_VERSION = -2,
+ ERROR_FAIL = -3,
+ ERROR_NI = -4,
+ ERROR_NOMEM = -5,
+ ERROR_INVAL = -6,
+ ERROR_BADFAIL = -7,
};
#define LIBXL_VERSION 0
diff -r 1d1eec7e1fb4 tools/libxl/libxl_utils.c
--- a/tools/libxl/libxl_utils.c Tue Jan 25 18:09:49 2011 +0000
+++ b/tools/libxl/libxl_utils.c Tue Jan 25 18:18:53 2011 +0000
@@ -93,7 +93,7 @@ int libxl_name_to_domid(libxl_ctx *ctx,
int i, nb_domains;
char *domname;
libxl_dominfo *dominfo;
- int ret = -1;
+ int ret = ERROR_INVAL;
dominfo = libxl_list_domain(ctx, &nb_domains);
if (!dominfo)
@@ -142,7 +142,7 @@ int libxl_name_to_cpupoolid(libxl_ctx *c
int i, nb_pools;
char *poolname;
libxl_cpupoolinfo *poolinfo;
- int ret = -1;
+ int ret = ERROR_INVAL;
poolinfo = libxl_list_cpupool(ctx, &nb_pools);
if (!poolinfo)
@@ -171,7 +171,7 @@ int libxl_name_to_schedid(libxl_ctx *ctx
if (strcmp(name, schedid_name[i].name) == 0)
return schedid_name[i].id;
- return -1;
+ return ERROR_INVAL;
}
char *libxl_schedid_to_name(libxl_ctx *ctx, int schedid)
@@ -287,7 +287,7 @@ int libxl_string_to_phystype(libxl_ctx *
} else if (!strcmp(s, "tap")) {
p = strchr(s, ':');
if (!p) {
- rc = -1;
+ rc = ERROR_INVAL;
goto out;
}
p++;
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] xl: avoid creating domains with duplicate names
2011-01-25 18:21 ` Ian Jackson
@ 2011-01-26 10:31 ` Stefano Stabellini
2011-01-26 12:00 ` Ian Jackson
0 siblings, 1 reply; 7+ messages in thread
From: Stefano Stabellini @ 2011-01-26 10:31 UTC (permalink / raw)
To: Ian Jackson; +Cc: xen-devel, Stefano Stabellini
On Tue, 25 Jan 2011, Ian Jackson wrote:
> Stefano Stabellini writes ("[Xen-devel] [PATCH] xl: avoid creating domains with duplicate names"):
> > Do not create the domain if another domain with the same name is already
> > running.
>
> Thanks. I approve of the principle of this patch, but:
>
> > + e = libxl_name_to_domid(&ctx, c_info->name, &domid_e);
> > + if (!e) {
>
> You should explicitly check the actual error return value of
> libxl_name_to_domid and check that it is the expected error code, and
> not some other error code meaning "general failure" or something.
>
> I went to look at the code for libxl_name_to_domid and it returns,
> entirely ad-hoc, -1 (which is now ERROR_VERSION), for "no such
> domain".
>
> IMO it should return ERROR_INVAL.
>
> I grepped the libxl source for "-1" and found that this practice is
> widespread. At this stage of the release I don't want to risk
> breaking everything by changing them all (since something may compare
> with -1, or something).
>
> So I suggest the attached fixup patch, and then a revised version of
> your patch. What do you think?
I think is a good idea.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] xl: avoid creating domains with duplicate names
2011-01-26 10:31 ` Stefano Stabellini
@ 2011-01-26 12:00 ` Ian Jackson
2011-01-26 12:07 ` Ian Jackson
0 siblings, 1 reply; 7+ messages in thread
From: Ian Jackson @ 2011-01-26 12:00 UTC (permalink / raw)
To: Stefano Stabellini; +Cc: xen-devel
Stefano Stabellini writes ("Re: [Xen-devel] [PATCH] xl: avoid creating domains with duplicate names"):
> On Tue, 25 Jan 2011, Ian Jackson wrote:
> > So I suggest the attached fixup patch, and then a revised version of
> > your patch. What do you think?
>
> I think is a good idea.
OK, thanks, I'll take that as an ack. I have applied my patch and
will post a revised version of yours on a moment.
Ian.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] xl: avoid creating domains with duplicate names
2011-01-26 12:00 ` Ian Jackson
@ 2011-01-26 12:07 ` Ian Jackson
2011-01-27 11:24 ` George Dunlap
0 siblings, 1 reply; 7+ messages in thread
From: Ian Jackson @ 2011-01-26 12:07 UTC (permalink / raw)
To: Stefano Stabellini, xen-devel
I wrote:
> OK, thanks, I'll take that as an ack. I have applied my patch and
> will post a revised version of yours on a moment.
TBH I think this restriction should be in libxl (in
libxl_domain_rename), rather than in xl. But that would require too
intrusive a set of changes at this stage.
xl: avoid creating domains with duplicate names
Do not create the domain if another domain with the same name is already
running.
This is another error-checking function at rather too high a level:
this should be moved into libxl_domain_rename in 4.2.
Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
diff -r 67d5b8004947 tools/libxl/xl_cmdimpl.c
--- a/tools/libxl/xl_cmdimpl.c Wed Jan 26 11:58:45 2011 +0000
+++ b/tools/libxl/xl_cmdimpl.c Wed Jan 26 12:02:37 2011 +0000
@@ -583,6 +583,7 @@ static void parse_config_data(const char
XLU_ConfigList *vbds, *nics, *pcis, *cvfbs, *net2s, *cpuids;
int pci_power_mgmt = 0;
int pci_msitranslate = 1;
+ uint32_t domid_e;
int e;
libxl_domain_create_info *c_info = &d_config->c_info;
@@ -612,6 +613,15 @@ static void parse_config_data(const char
if (xlu_cfg_replace_string (config, "name", &c_info->name))
c_info->name = strdup("test");
+ e = libxl_name_to_domid(&ctx, c_info->name, &domid_e);
+ if (!e) {
+ fprintf(stderr, "A domain with name \"%s\" already exists.\n", c_info->name);
+ exit(1);
+ }
+ if (e != ERROR_INVAL) {
+ fprintf(stderr, "Unexpected error checking for existing domain"
+ " (error=%d)", e);
+ }
if (!xlu_cfg_get_string (config, "uuid", &buf) ) {
if ( libxl_uuid_from_string(&c_info->uuid, buf) ) {
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] xl: avoid creating domains with duplicate names
2011-01-26 12:07 ` Ian Jackson
@ 2011-01-27 11:24 ` George Dunlap
2011-01-27 19:49 ` Ian Jackson
0 siblings, 1 reply; 7+ messages in thread
From: George Dunlap @ 2011-01-27 11:24 UTC (permalink / raw)
To: Ian Jackson; +Cc: xen-devel, Stefano Stabellini
Looks like this patch is causing failures in guest-localmigrate:
2011-01-27 03:06:09 Z executing ssh ... root@10.80.249.56 xl migrate
debian.guest.osstest localhost
migration target: Ready to receive domain.
Saving to migration stream new xl format (info 0x0/0x0/774)
Loading new save file incoming migration stream (new xl fmt info 0x0/0x0/774)
Savefile contains xl domain config
A domain with name "debian.guest.osstest" already exists.
xc: error: write: p2m_size (32 = Broken pipe): Internal error
libxl: error: libxl_dom.c:444:libxl__domain_suspend_common saving
domain: Broken pipe
migration sender: libxl_domain_suspend failed (rc=-3)
libxl: info: libxl_exec.c:70:libxl_report_child_exitstatus migration
target process [20216] exited with error status 1
Migration failed, resuming at sender.
Do we need to special-case local migration?
-George
On Wed, Jan 26, 2011 at 12:07 PM, Ian Jackson <Ian.Jackson@eu.citrix.com> wrote:
> I wrote:
>> OK, thanks, I'll take that as an ack. I have applied my patch and
>> will post a revised version of yours on a moment.
>
> TBH I think this restriction should be in libxl (in
> libxl_domain_rename), rather than in xl. But that would require too
> intrusive a set of changes at this stage.
>
>
> xl: avoid creating domains with duplicate names
>
> Do not create the domain if another domain with the same name is already
> running.
>
> This is another error-checking function at rather too high a level:
> this should be moved into libxl_domain_rename in 4.2.
>
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
>
> diff -r 67d5b8004947 tools/libxl/xl_cmdimpl.c
> --- a/tools/libxl/xl_cmdimpl.c Wed Jan 26 11:58:45 2011 +0000
> +++ b/tools/libxl/xl_cmdimpl.c Wed Jan 26 12:02:37 2011 +0000
> @@ -583,6 +583,7 @@ static void parse_config_data(const char
> XLU_ConfigList *vbds, *nics, *pcis, *cvfbs, *net2s, *cpuids;
> int pci_power_mgmt = 0;
> int pci_msitranslate = 1;
> + uint32_t domid_e;
> int e;
>
> libxl_domain_create_info *c_info = &d_config->c_info;
> @@ -612,6 +613,15 @@ static void parse_config_data(const char
>
> if (xlu_cfg_replace_string (config, "name", &c_info->name))
> c_info->name = strdup("test");
> + e = libxl_name_to_domid(&ctx, c_info->name, &domid_e);
> + if (!e) {
> + fprintf(stderr, "A domain with name \"%s\" already exists.\n", c_info->name);
> + exit(1);
> + }
> + if (e != ERROR_INVAL) {
> + fprintf(stderr, "Unexpected error checking for existing domain"
> + " (error=%d)", e);
> + }
>
> if (!xlu_cfg_get_string (config, "uuid", &buf) ) {
> if ( libxl_uuid_from_string(&c_info->uuid, buf) ) {
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] xl: avoid creating domains with duplicate names
2011-01-27 11:24 ` George Dunlap
@ 2011-01-27 19:49 ` Ian Jackson
0 siblings, 0 replies; 7+ messages in thread
From: Ian Jackson @ 2011-01-27 19:49 UTC (permalink / raw)
To: George Dunlap; +Cc: xen-devel, Stabellini
George Dunlap writes ("Re: [Xen-devel] [PATCH] xl: avoid creating domains with duplicate names"):
> Looks like this patch is causing failures in guest-localmigrate:
Thanks. See my series of 6 small patches to fix this. Following
consultations, I have already applied the first, which was simply to
revert the check for now pending a proper fix.
Ian.
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2011-01-27 19:49 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-01-25 15:15 [PATCH] xl: avoid creating domains with duplicate names Stefano Stabellini
2011-01-25 18:21 ` Ian Jackson
2011-01-26 10:31 ` Stefano Stabellini
2011-01-26 12:00 ` Ian Jackson
2011-01-26 12:07 ` Ian Jackson
2011-01-27 11:24 ` George Dunlap
2011-01-27 19:49 ` Ian Jackson
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.